Skip to content

fix(bin): refuse stale pooled worktree ownership - #3246

Open
jleemcf wants to merge 48 commits into
kunchenguid:mainfrom
jleemcf:fm/fm-teardown-stale-worktree-ownership-v2
Open

fix(bin): refuse stale pooled worktree ownership#3246
jleemcf wants to merge 48 commits into
kunchenguid:mainfrom
jleemcf:fm/fm-teardown-stale-worktree-ownership-v2

Conversation

@jleemcf

@jleemcf jleemcf commented Aug 28, 2026

Copy link
Copy Markdown

Summary

  • Add one shared worktree-ownership resolver used by teardown, relaunch, and recursive recovery cleanup before they act on a recorded path.
  • Stamp each spawned worktree with a git-excluded Firstmate owner marker bound to both task id and spawn generation; refuse a slot marked for another task without overwriting or deleting that marker.
  • Retire the marker and worktree= claim before returning or removing a slot, then use a non-authoritative retirement receipt so late cleanup failures can be retried without restoring destructive authority over a recycled path.
  • Preserve the existing dirty and unlanded-work checks, support unbranched scouts and pre-branch ships, and keep task-branch cleanup explicit and bounded to fm/<task-id>.

Issue impact

This change makes destructive lifecycle actions refuse when ownership has moved. It does not stop the pool from handing the same slot out twice; that root cause is deliberately out of scope and is filed separately as a follow-up.

Validation

  • Ownership regressions cover the original destroy-the-wrong-worker incident and verify that the live task's uncommitted work survives.
  • An exact detached, unbranched scout is positively protected by its task-and-generation owner marker.
  • Fresh-spawn, relaunch, teardown, recursive secondmate cleanup, Orca, tmux, Zellij, and backend-dispatch suites pass.
  • Late teardown failure followed by a successful retry is covered without restoring worktree= or .fm-task-owner after provider release.
  • Existing dirty-worktree and unlanded-commit refusal coverage remains passing.
  • Repository-pinned ShellCheck 0.11.0, actionlint 1.7.12, workflow lint, and documentation-audience checks pass.

Updates from git push no-mistakes

@greptile-apps

greptile-apps Bot commented Aug 28, 2026

Copy link
Copy Markdown

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Reviews (3): Last reviewed commit: "test(spawn): use distinct slots and prov..." | Re-trigger Greptile

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bb948d695b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread bin/fm-spawn.sh Outdated
printf '%s' 'a marker with no readable task identity'
return 1
fi
[ "$owner" != "$ID" ] || return 0

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Refuse same-task owner markers on fresh spawns

Captain, when a fresh recovery spawn reuses a task ID after its pending ownership record was lost or became unreadable, this check accepts an existing marker solely because task_id matches; stamp_task_worktree_owner then overwrites its generation and can launch a second worker into a worktree still owned by the earlier incarnation. Only relaunch has metadata and handoff evidence that can safely accept a same-ID marker, so a fresh spawn must refuse any existing marker rather than treating the ID match as ownership proof.

AGENTS.md reference: AGENTS.md:L228-L229

Useful? React with 👍 / 👎.

@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate:

Reviewed HEAD bb948d695ba840681d04f6e263248be29029d831 vs main. Whole thread read (Greptile 5/5; Codex P1 on fresh-spawn same-task markers). jleemcf is not blocked. Not flagged-malicious. Compare to main shows zero .github/workflows/* file changes. No sudo / pull_request_target / privilege widening.

Class: corrective. Destructive teardown/relaunch/recovery currently trust a recorded pool path that may have been reissued. This adds a task+generation owner marker and a shared ownership resolver so those actions refuse when the slot belongs to someone else. Captain-facing spawn/teardown stay the same; they just stop destroying the wrong worker. Does not stop the pool from handing a slot out twice (author correctly left that out of scope).

VISION.md per-rule (inspected bin/fm-worktree-ownership-lib.sh, bin/fm-spawn.sh stamp/refuse, bin/fm-teardown.sh proof + retirement receipt, bin/fm-control.sh, bin/fm-backend.sh, tests/fm-teardown.test.sh / tests/fm-secondmate-safety.test.sh / tests/fm-control-relaunch.test.sh):

  • One captain, one interface — aligns (ownership proof stays below deck).
  • Authority is explicit — aligns (refuses destructive cleanup when ownership moved; no new autonomy grant).
  • Scripts own the mechanics — aligns (marker + receipt resolver; jq-optional fail-closed).
  • A restart is a non-event — aligns (receipts let a partial teardown finish without restoring destructive authority over a recycled path).
  • Delegation with a spine — aligns (existing spawn/teardown primitive + ownership regressions).
  • The fleet outlives any vendor — aligns (backend-agnostic marker; Orca/tmux/Zellij coverage).
  • Scope — aligns (lifecycle safety; not a forge/CI change).

Attestation: missing. No no-mistakes-pipeline-attestation:v1 head_sha bind on this HEAD.

Codex P1 (waiting on author): a fresh recovery spawn that reuses a task id after a lost/unreadable pending-ownership record accepts an existing marker solely because task_id matches, then stamp_task_worktree_owner overwrites its generation and can launch a second worker into a still-owned worktree. Fresh spawn should refuse any existing marker; only relaunch has the metadata to accept a same-id handoff.

Fork CI: approved this pass on this HEAD only: 33191534804 (CI), 33191534815 (Require no-mistakes).

Overlap note (not a hold): open #3168 / #3161 also touch bin/fm-spawn.sh / bin/fm-teardown.sh, but they are process-reap / lsof-fallback work, not this ownership-proof slice.

Waiting on the author for attestation + the fresh-spawn marker hole. Not a captain-decision. No auto-merge.

@jleemcf
jleemcf force-pushed the fm/fm-teardown-stale-worktree-ownership-v2 branch from bb948d6 to dd87a73 Compare September 2, 2026 08:55
@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate:

Re-reviewed after newer activity (prior stamp was on bb948d69; HEAD is now dd87a736de6cddbe75f9bc24065de5584e12a828). Whole thread re-read (Greptile 5/5 on this HEAD; Codex P1 on the old commit). jleemcf is not blocked. Not flagged-malicious. Compare to main shows zero .github/workflows/* file changes. No sudo / pull_request_target / privilege widening.

Codex P1 (prior wait): addressed on this HEAD. task_worktree_owner_marker_holder in bin/fm-spawn.sh now accepts a same-task marker only when RELAUNCH=1; a fresh spawn refuses every existing marker (foreign, unreadable, or same-id).

Contract-class: new-default. Unconfigured new ordinary claims always stamp .fm-task-owner and set task_owner_marker=1, and destructive lifecycle paths prove ownership through bin/fm-worktree-ownership-lib.sh before acting. Main trusted the recorded worktree= claim (with legacy branch/project fallbacks); this replaces that default path with always-on marker-backed proof for new marker-aware records. Legacy records without the bit keep the old fallbacks. Not restore (bug-motive alone does not restore); not opt-in. No auto-merge.

VISION.md per-rule (inspected bin/fm-worktree-ownership-lib.sh prove/retire/receipt, bin/fm-spawn.sh stamp/refuse + fresh-vs-relaunch, bin/fm-teardown.sh retirement preflight, bin/fm-control.sh / bin/fm-backend.sh call sites, ownership docs in docs/architecture.md, regressions in tests/fm-teardown.test.sh / tests/fm-secondmate-safety.test.sh / tests/fm-control-relaunch.test.sh):

  • One captain, one interface — aligns (ownership proof stays below deck; captain-facing spawn/teardown verbs unchanged).
  • Authority is explicit — aligns (refuses destructive cleanup when ownership moved; no new autonomy grant).
  • Scripts own the mechanics — aligns (marker + pending record + receipt resolver; jq-optional fail-closed).
  • A restart is a non-event — aligns (pending ownership records + retirement receipts survive partial teardown without restoring destructive authority over a recycled path).
  • Delegation with a spine — aligns (strengthens refuse path for wrong-slot destroy; existing spawn/teardown primitives).
  • The fleet outlives any vendor — aligns (backend-agnostic marker; Orca/tmux/Zellij coverage retained).
  • Scope — aligns (lifecycle safety; not a forge/CI change). Closing: deepens refusal safety; resist only if always-on marker path were treated as silent consent — it is not; it is fail-closed refuse.

Attestation: MATCH (no-mistakes-pipeline-attestation:v1 head_sha=dd87a736de6cddbe75f9bc24065de5584e12a828 == HEAD).

Fork CI: approved this pass on this HEAD: 33611362366 (CI), 33611361925 + 33611397247 (Require no-mistakes).

CI/NM on this HEAD: NM 33611397247 SUCCESS. CI 33611362366 FAILURE so far:

  • Behavior tests (Herdr): not ok - fm-hibit-resume-r1 same-identity reclaim failed — fresh spawn into a slot still marked for task anchor is refused by the new owner-marker gate (worktree ... already belongs to task anchor; ... a fresh spawn refuses any existing owner marker).
  • Behavior portable serial 1: not ok - spawn with absolute symlink-spelled FM_HOME defaults should succeed in tests/fm-spawn-dispatch-profile.test.sh (exit 1).

Waiting on the author to clear those CI failures (fixture/relaunch path vs marker lifecycle). Not a captain-decision while CI is red. No auto-merge.

Overlap note (not a hold): several open PRs also touch bin/fm-spawn.sh / bin/fm-teardown.sh (e.g. #3511 lifecycle hardening, #3483 task tmp roots, #3457 scratch teardown); none is this ownership-proof slice.

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