fix(fm-teardown): open own-PR vetoes the content-in-default landed check#988
Open
jokim1 wants to merge 1 commit into
Open
fix(fm-teardown): open own-PR vetoes the content-in-default landed check#988jokim1 wants to merge 1 commit into
jokim1 wants to merge 1 commit into
Conversation
work_is_landed() fell back to content_in_default() whenever the task's own PR was not merged. That fallback only asks "is this branch's content already in the default branch?" via tree-equality, which is true both when the task's own PR squash-merged (intended) and when a SIBLING PR carried the same content while this task's own PR is still open (false positive). A normal teardown then deleted the task's state records (.meta/.status/.turn-ended/PR-poll sidecars) while its PR was live - reaping three real tasks on 2026-07-22. An open PR is affirmative proof the task's own work has not landed. When a pr= is recorded and GitHub reports it OPEN, work_is_landed now refuses without consulting content_in_default. The content fallback is unchanged for tasks with no recorded PR, a gh error, or a merged PR (squash-then-delete still works). The refusal now tells the operator the PR is still open. Extends tests/fm-teardown.test.sh with the sibling-content scenarios: recorded OPEN PR + content in default -> REFUSE; recorded MERGED PR + content in default -> ALLOW. The existing no-PR + content-in-default case is unchanged. Refs kunchenguid#986
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
bin/fm-teardown.sh's landed-work safety (work_is_landed->content_in_default) can delete a ship task'sstate/<id>.meta,.status,.turn-ended, and PR-poll/check sidecars while that task's own PR is still open (green but unmerged).content_in_default()(bin/fm-teardown.sh:385) only checks whether the branch's content is already in the up-to-date default branch, viagit merge-tree --write-treetree-equality. That is true both when the task's own PR squash-merged (intended) and when a sibling PR landed the same/overlapping content first (false positive). Becausework_is_landedwaspr_is_merged || content_in_default, an open PR did not stop the fallback, so a normal (no--force) teardown proceeded and reaped live work.Impact (observed)
Three tasks on 2026-07-22 in one home lost
.meta+.statusafter reaching done, with no intended teardown, all shared-surface slices:rb-audit-surface-data-2a(PR fix(dashboard): restore accurate fleet pipeline lanes #680) - slice A of a shared audit-surfacing contract with slice B, sharing types/RPC/queries.ct-catalog-index-fix-4g- carried two PRs (test(orca): stub claude in Orca spawn fixtures so Linux CI clears preflight #800/feat(bootstrap): surface fork upstream drift at session start #801); the earlier PR's content already in main made the delivered branch's content match the default tree.rb-agent-auth-audit-6a- an audit scout.Recovery required hand-rewriting
.metaand re-runningfm-pr-checkbefore the still-open PRs could be merged. The watcher's.seen-*/.hb-surfaced-*markers survived with the same mtimes as the removed files, ruling out an age-based external reaper orgit clean; the exact removed set matches teardown's ownrmlist. The mechanism was reproduced end to end against the realbin/fm-teardown.sh(open, unmerged PR + sibling content in default -> teardown removed.meta/.status/.turn-ended, exit 0, no REFUSED).Fix
An open PR is affirmative proof the task's own work has not landed, so
content_in_default(a fallback for the unknown/absent PR-state case) must not override it.In
work_is_landed(): when apr=is recorded and GitHub reports it OPEN, refuse without consultingcontent_in_default. The content fallback is unchanged for tasks with no recorded PR, a gh lookup error, or a merged PR (squash-merge-then-delete still works). The refusal now tells the operator the PR is still open.Test coverage
Extends
tests/fm-teardown.test.shwith the sibling-content scenarios:pr=reported OPEN + content already in default via a sibling -> REFUSE (new open-PR veto).pr=reported MERGED + content in default -> ALLOW (own PR genuinely landed; veto must not over-refuse).test_content_in_default_fallback_allows) is unchanged.All teardown cases pass;
bin/fm-lint.shis clean (shellcheck 0.11.0, the pinned version).Fixes #986