Skip to content

fix(bin): refuse to hand out or release a worktree another task owns - #3519

Open
jericho1050 wants to merge 1 commit into
kunchenguid:mainfrom
jericho1050:fm/fm-two-lanes-one-worktree
Open

fix(bin): refuse to hand out or release a worktree another task owns#3519
jericho1050 wants to merge 1 commit into
kunchenguid:mainfrom
jericho1050:fm/fm-two-lanes-one-worktree

Conversation

@jericho1050

Copy link
Copy Markdown

Why

A task that has FINISHED but whose PR is UNMERGED is correctly never torn down, because teardown requires confirmed landing. Its worker stops, its pane closes, and its recorded worktree stays claimed while looking idle to everything except its own state/<id>.meta. The pool allocator then hands that same directory to the next spawn.

This happened FOUR times in one day. The sharpest instance: a cost investigation was handed the exact copy another task was mid-merge in, with two unresolved files and a staged conflict resolution that existed nowhere else. It was caught by attention, not by anything structural.

No work was lost on any occasion, because each finished task's branch was already on the server. But the hazard is asymmetric: tearing down the finished task would return the directory from under a live worker mid-run.

What this does

Two refusals, never repairs. Neither guard releases, reassigns, switches a branch, or cleans anything up - a guard that acts is a guard that can destroy.

  • fm-spawn.sh refuses to record a worktree another live state/*.meta already names, and says which task holds it.
  • fm-teardown.sh refuses when the recorded copy is checked out on a different task's branch, naming that branch and task.

Tests

tests/fm-worktree-ownership.test.sh, 4 cases, all passing:

  • fm-spawn refuses a worktree already named by another task
  • fm-spawn still succeeds on a genuinely free worktree (control)
  • fm-teardown refuses a copy checked out on another task's branch
  • fm-teardown still succeeds for a copy on its own task branch (control)

The two controls are load-bearing: without them, "correctly refuses collisions" and "refuses everything" are indistinguishable.

Disclosure

The authoring worker went unreachable after committing - its composer wedged, so it could neither push nor open this PR. Firstmate ran the test suite itself to verify what it was preserving, then pushed the branch and opened this PR by hand. No code was changed after the worker's commit.

Consequently the "PR must be raised via no-mistakes" check will fail, and that failure is honest rather than incidental: this PR genuinely was not raised through that pipeline.

@greptile-apps

greptile-apps Bot commented Sep 2, 2026

Copy link
Copy Markdown

Confidence Score: 5/5

The PR appears safe to merge, with no concrete changed-code failure identified.

The new checks stop before mutation, preserve ordinary free-worktree and own-branch behavior, and operate within the existing serialized spawn lifecycle.

Reviews (1): Last reviewed commit: "Guard worktree ownership during spawn an..." | Re-trigger Greptile

@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate: full-thread + full-diff review of #3519 (jericho1050). Author is not on the blocked list.

Attestation: MISSING. PR body has no <!-- no-mistakes-pipeline-attestation:v1 {"head_sha":"..."} --> for HEAD be671c7cd69bc76888b7e2e09e3e200a6f3d6ead. Author disclosure is honest: this tip was pushed/opened by hand after the authoring worker wedged, so Require no-mistakes run 33631599005 FAILED ("PR must be raised via no-mistakes") as expected. Re-submit via git push no-mistakes so attestation MATCHES this (or a rebased) tip before any merge consideration.

First-time fork CI this pass: after safe diff review (zero .github/workflows/* changes; no secrets/pull_request_target/credential paths; guards refuse only — no release/reassign/cleanup mutation), approved workflow runs 33631598911 (CI) and 33631599005 (Require no-mistakes). CI is in_progress on this HEAD; early jobs Repo invariants + Test coverage guard already SUCCESS. Greptile SUCCESS 5/5. MERGEABLE / UNSTABLE vs main 8988af2a (ahead 1 / behind 0).

Contract-class: new-default. Unconfigured spawn now always runs spawn_refuse_worktree_collision (refuse recording a worktree another live state/*.meta already names, with dead/missing holders still claimed when the copy is on fm/<holder>), and teardown always runs teardown_refuse_if_other_task_branch before any mutation. Main handed out / tore down from the recorded path without these always-on collision refusals. Not restore; not opt-in. No auto-merge. Firstmate escalate only when otherwise ready (CLEAN + MATCH + green CI/NM + safe) — not this pass (attestation + NM blocked).

VISION.md per-rule (inspected bin/fm-spawn.sh spawn_refuse_worktree_collision, bin/fm-teardown.sh teardown_refuse_if_other_task_branch, tests/fm-worktree-ownership.test.sh refuse + control cases):

  • One captain, one interface — aligns. Ownership collisions stay below deck; captain sees a named refuse with next step.
  • Authority is explicit and never inferred — aligns. Refuses destructive/reassignment risk; no new autonomy grant; no --force bypass of the teardown branch guard.
  • Scripts own the mechanics, agents own the judgment — aligns. Deterministic meta/path/branch checks; no agent adjudicates ownership.
  • A restart is a non-event — aligns. Finished-but-unmerged holders keep their claim via meta + task branch even after endpoint death.
  • Delegation with a spine — aligns. Strengthens refuse path; unlanded work is not torn down from under another lane.
  • The fleet outlives any vendor — aligns. Backend-agnostic via meta + git branch (agent_state only softens dead/missing holder retention).
  • Scope — aligns. Lifecycle safety only; not a forge/CI change.

Security: clean — refusals only; no workflow/RCE/credential surface; no ownership bypass. Not Firstmate-flagging.

Overlap note (not a hold): open #3246 (jleemcf, already stamped waiting-author new-default) also refuses stale pooled worktree ownership via always-on .fm-task-owner + task_owner_marker=1 and a shared ownership lib, with CI still red on this morning's tip. #3519 is a thinner meta/branch collision refuse without the marker/receipt machinery. Same hazard class; different mechanism. Captain should pick one path (or compose deliberately) before merging either as the default.

Waiting on author for attestation (NM green) and green CI. Not escalating; not merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants