From 51832b3afb60020ba2c28337631de156ef43d61d Mon Sep 17 00:00:00 2001 From: Daniel Petty Date: Sun, 8 Nov 2026 04:58:51 +0000 Subject: [PATCH 1/4] test(roadmap): add tests for the orphan draft of a failed stage marks write Plans #4051. Adds the OpenSpec change and the key stage.pending that the tests import. --- .../.openspec.yaml | 2 + .../design.md | 21 + .../proposal.md | 58 +++ .../specs/roadmap-stage/spec.md | 49 ++ .../tasks.md | 40 ++ src/devops_cli/roadmap/stores/store.py | 1 + .../stage/test_roadmap_stage_orphan.py | 460 ++++++++++++++++++ 7 files changed, 631 insertions(+) create mode 100644 openspec/changes/4051-remove-the-orphan-draft-that-a/.openspec.yaml create mode 100644 openspec/changes/4051-remove-the-orphan-draft-that-a/design.md create mode 100644 openspec/changes/4051-remove-the-orphan-draft-that-a/proposal.md create mode 100644 openspec/changes/4051-remove-the-orphan-draft-that-a/specs/roadmap-stage/spec.md create mode 100644 openspec/changes/4051-remove-the-orphan-draft-that-a/tasks.md create mode 100644 tests/roadmap/stage/test_roadmap_stage_orphan.py diff --git a/openspec/changes/4051-remove-the-orphan-draft-that-a/.openspec.yaml b/openspec/changes/4051-remove-the-orphan-draft-that-a/.openspec.yaml new file mode 100644 index 000000000..c0ad5ed4d --- /dev/null +++ b/openspec/changes/4051-remove-the-orphan-draft-that-a/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-11-08 diff --git a/openspec/changes/4051-remove-the-orphan-draft-that-a/design.md b/openspec/changes/4051-remove-the-orphan-draft-that-a/design.md new file mode 100644 index 000000000..620bc57ec --- /dev/null +++ b/openspec/changes/4051-remove-the-orphan-draft-that-a/design.md @@ -0,0 +1,21 @@ +# Design + +## Decisions + +- **Write an intent mark before the push.**: The mark `stage.pending` holds the branch name and the time. The job can then tell its own orphan from a branch of a person or of an agent, which has no mark. +- **Store the mark as JSON with the fields `branch` and `time`, as `stage.rejected` does.**: The pass uses the branch of the mark. A retitle between the failure and the next round changes the slug of the branch, but not the mark. +- **Write the mark with the `now` of the `StageWriter`.**: The tests set the clock, and the Service passes its own clock. The pass in `plan_stage` uses the `now` of the plan to judge the age. +- **Judge the orphans in `plan_stage`, like `_reconcile`, and read the open pull requests in the writer.**: `plan_stage` stays read-only, and a dry run makes no request. The plan holds a `Removal` for each orphan. `StageWriter.begin` reads `store.open_pulls()` once, only when a removal with a pushed branch exists, and finds the draft by its head. +- **Keep the age as the constant `CONST_ROADMAP_STAGE_ORPHAN_AGE_SECONDS` of 3600 seconds.**: A run in another clone can be in the middle of its publish. A setting would add a key to the ConfigMap for a value that nobody needs to tune. +- **Judge a mark that is as old as the limit.**: The rule is "younger than 1 hour is left alone", so a mark of exactly 1 hour is judged. +- **Judge the pass after the selection of the round.**: The item is still held by its pushed branch in the round that removes the branch. The next round plans it again. The plan facts (the groups with measured tokens and the hash of the text) were in the marks write that failed, so the planner must run again. +- **A bad record of the mark counts as no mark.**: A value that is not JSON, or that holds no branch or time, is logged and the pass leaves the item alone, as `recorded_rejection` does. +- **Do not widen to a closed item.**: A closed item that holds `stage.pending` and no `stage.pr` is not in the issue. It is a known gap. + +## Rejected alternatives + +- **Delete every pushed branch of an open item that has no `stage.pr`, found by issue number and branch pattern.**: `issue_holder` in `src/devops_cli/git/worktree_add.py` finds a branch by the issue number only. `OpenPull` in `src/devops_cli/roadmap/stores/store.py` has no author field, and `_foreign_commits` compares the commit authors with the `user.name` of the clone. A clone that has the identity of a person passes the guard, so the rule could delete the work of a person or of another agent. It would also defeat `_Judge._held`. +- **Write the branch in `stage.branch` before the push and use that key.**: `_reconcile` and the Service read `stage.branch` as the record of a finished stage. An early write would make a half-staged item look staged. +- **Search the pushed branches for the author `devops-roadmap[bot]`.**: The clone commits as the identity of the App, so a branch of an agent in the same clone has the same author. The mark is the only proof of intent. +- **Order the steps differently so that the marks write comes before the draft.**: The marks write needs the number of the draft pull request, and the record must not name a draft that does not exist. +- **Read the open pull requests in `plan_stage`.**: This adds a request to every round, and a dry run must make none. diff --git a/openspec/changes/4051-remove-the-orphan-draft-that-a/proposal.md b/openspec/changes/4051-remove-the-orphan-draft-that-a/proposal.md new file mode 100644 index 000000000..2d0d16ff1 --- /dev/null +++ b/openspec/changes/4051-remove-the-orphan-draft-that-a/proposal.md @@ -0,0 +1,58 @@ +# Proposal + +## Why + +The issue #4051 reports an orphan draft. `_stage_one` in `src/devops_cli/roadmap/stage/stage.py` pushes the branch, opens the draft pull request, and then writes the marks in one `set_marks` call. When that write fails, or when the worker dies between the push and the write, the branch and the draft stay on GitHub and the item records no `stage.pr`. The item then stays out of every later round, because `_Judge._held` holds an item for any pushed branch that names its issue number. `_reconcile` reads closed items only. A person must close the draft and delete the branch by hand. + +A rule that deletes every pushed branch of an open item with no `stage.pr` is not safe. A branch of a person or of an agent for the same issue number looks the same, and `OpenPull` has no author field. The job needs a mark that proves the branch is its own. + +## What Changes + +- Add the key `stage.pending` to `StageRecordKey` in `src/devops_cli/roadmap/stores/store.py`. Its value is JSON with the branch name and the time: `{"branch": "", "time": ""}`. +- `_stage_one` writes `stage.pending` with one `set_marks` before the push. The time is the `now` of the `StageWriter`. The marks write after the draft sets `stage.pending` to None with the other keys. +- Add a pass `_orphans` next to `_reconcile`. `plan_stage` calls it with its `now` and adds its results to `StagePlan.removals` and `StagePlan.kept`. The pass reads the open items that have `stage.pending` and no `stage.pr`, and it uses the branch name of the mark: + - A mark younger than a constant age of 1 hour gives nothing. The age is the constant `CONST_ROADMAP_STAGE_ORPHAN_AGE_SECONDS` in `src/devops_cli/config/constants.py`, and no setting. + - A branch that is not on `origin` gives `Removal(..., gone=True)`: only the record is cleared. + - A pushed branch with `_foreign_commits` equal to 0 gives a `Removal`. The writer reads `store.open_pulls()` once per round, only when such a removal exists, closes the open pull request whose head is the branch, deletes the branch and clears the record. + - A pushed branch with a commit of another author gives a `KeptBranch`, and costs no request. +- A pushed branch with no `stage.pending` mark is never touched. +- The dry run plan (`stage_requests` in `src/devops_cli/roadmap/request_plan.py`) lists the write of the mark before the push and the read of the open pull requests. +- Fix the documents that the new key makes wrong: `docs/ROADMAP_WORKFLOW.md` (the table of keys), `docs/roadmap/stage.md` (the orphan pass) and the docstring of `StageRecordKey`. +- Add a requirement to the `roadmap-stage` capability, because the stage job removing an orphan is behavior that a consumer observes. +- No setting is added, renamed, removed or tightened. The example and the live ConfigMap need no change. The test `tests/k8s` key-walk test of #4093 still proves that the example ConfigMap loads into `Settings`. +- The change fits one pull request. + +## Capabilities + +### New Capabilities + +None. + +### Modified Capabilities + +- `roadmap-stage`: A failed marks write leaves no orphan draft + +## Impact + +- Code: `src/devops_cli/roadmap/stage/stage.py`, `src/devops_cli/roadmap/stores/store.py`, `src/devops_cli/roadmap/request_plan.py`, `src/devops_cli/config/constants.py`, `src/devops_cli/lang/en/messages.py` +- Tests: `tests/roadmap/stage/test_roadmap_stage_orphan.py` +- Documents: `docs/ROADMAP_WORKFLOW.md`, `docs/roadmap/stage.md` +- Request cost: each staged item adds one `set_marks` call (2 requests: one read and one PATCH). A round with an orphan to remove adds one read of the open pull requests. Other rounds add none. +- Open point for review: the owner has not yet confirmed the intent-mark approach. It replaced the plain rule "delete the pushed branch by issue number and branch pattern", because that rule could delete the branch of a person or of another agent. +- Known gap, not widened: a closed item that holds `stage.pending` and no `stage.pr` is not an orphan of this pass. `_reconcile` clears a closed item only when it holds `stage.branch`. + +## Feasibility + +I checked the premise at `origin/main` by reading the code. I ran no GitHub request. + +- `src/devops_cli/roadmap/stage/stage.py`: `_stage_one` calls `_commit_and_push`, then `_open_pull`, then one `set_marks` with BRANCH, PR, SHA, GROUPS and TEXT_HASH, then the labels, then the comment. `_open_pull` deletes the branch when the draft fails. `_label_pull` and `_label_needs_fix` catch `GitHubOperationError`. So only a failed marks write, or a death of the worker between the push and the write, leaves a pushed branch and a draft with no `stage.pr`. +- `_reconcile` reads the closed items with `stage.branch`. `_Judge._held` uses `issue_holder` in `src/devops_cli/git/worktree_add.py`, which holds an item for any pushed branch named with the issue number. This confirms that the item stays out of every round. +- `OpenPull` in `src/devops_cli/roadmap/stores/store.py` has `head_ref`, `number` and `draft`, and no author. `_foreign_commits` is the only author guard. This confirms why the plain rule is unsafe. +- `_remove_staged` clears `{key: None for key in StageRecordKey}`, so a new key is cleared with the rest. `RejectedPlan` in `src/devops_cli/roadmap/stage/stage_rejected.py` is the pattern for a JSON mark. +- `set_marks` in `src/devops_cli/roadmap/stores/github_store.py` makes one read and one PATCH, so the cost of 2 requests holds. +- The tests in `tests/roadmap/stage/test_roadmap_stage_orphan.py` run on the unchanged code, and 11 of 17 fail for the reasons above (`devops ci fail-to-pass --base origin/main`). The 6 that pass are guards: they prove that a mark of a young age, a recorded pull request and a closed item are left alone. +- I did not check how GitHub treats the open pull request of a deleted head branch. The pass closes the draft itself, so it does not depend on that behavior. + +## Existing Tool Considered + +No open, maintained tool or library repairs the state of one job in the record of an issue. The change reuses `_remove_staged`, `Removal`, `KeptBranch`, `_foreign_commits` and the JSON mark pattern of `stage.rejected`, all in the repository. The change builds one key, one write and one pass. The written cost: about 50,000 tokens and 2.5 hours to build, about 2,000 tokens each quarter to keep, and about 10,000 tokens and 0.5 hour to fix. No internal code is deleted. diff --git a/openspec/changes/4051-remove-the-orphan-draft-that-a/specs/roadmap-stage/spec.md b/openspec/changes/4051-remove-the-orphan-draft-that-a/specs/roadmap-stage/spec.md new file mode 100644 index 000000000..3e1f00c4d --- /dev/null +++ b/openspec/changes/4051-remove-the-orphan-draft-that-a/specs/roadmap-stage/spec.md @@ -0,0 +1,49 @@ +## ADDED Requirements + +### Requirement: A staged item records its intent before the push +The job SHALL write the mark `stage.pending`, the branch name and the time as JSON, with one write of marks before it pushes the branch of an item. The write of the marks after the draft SHALL clear `stage.pending` with the other keys. + +#### Scenario: The request cost of a staged item +- **WHEN** a round stages one item +- **THEN** the round writes the marks twice for that item, and the record holds no `stage.pending` at the end + +### Requirement: An old intent mark with no recorded pull request gives an orphan +In each run the job SHALL look at the open items that hold `stage.pending` and no `stage.pr`, and SHALL use the branch name of the mark. A mark younger than 1 hour SHALL give no action. A branch that is not on `origin` SHALL give only the clear of the record. + +#### Scenario: The marks write fails +- **WHEN** the write of the marks after the draft fails, and a later round runs after the mark is 1 hour old +- **THEN** the round closes the draft, deletes the branch and clears the record, and a following round plans the item again + +#### Scenario: The worker dies after the push +- **WHEN** the worker dies after the push and before the draft, and a later round runs after the mark is 1 hour old +- **THEN** the round deletes the branch, makes no request to close a draft and clears the record + +#### Scenario: A retitle changes the slug +- **WHEN** a person changes the title of the item between the failure and the next round +- **THEN** the round still finds the orphan by the branch name of the mark + +#### Scenario: A young mark +- **WHEN** the mark is younger than 1 hour +- **THEN** the job makes no request and keeps the mark, and a round that runs when the mark is 1 hour old judges it + +#### Scenario: The branch is gone +- **WHEN** the branch of the mark is not on `origin` +- **THEN** the job clears the record and makes no request + +### Requirement: The job removes an orphan only when it wrote the branch alone +When every commit of the pushed branch of an orphan past the default branch is by the identity of the clone or is a clean merge, the job SHALL close the open pull request whose head is the branch, delete the branch and clear the record. The job SHALL read the open pull requests once in a round, and only when such a branch exists. When any other commit exists, the job SHALL keep the branch and the draft and say so in the plan. + +#### Scenario: A person added a commit +- **WHEN** the pushed branch of the mark holds a commit by another author +- **THEN** the job keeps the branch and the draft, makes no request and the plan says why + +#### Scenario: The request cost of a removal +- **WHEN** a round removes two orphans +- **THEN** the round reads the open pull requests once + +### Requirement: A branch with no intent mark is never touched +The job SHALL NOT close, delete or change a pushed branch or a pull request whose item holds no `stage.pending`. + +#### Scenario: A branch with no mark +- **WHEN** a person or an agent pushed a branch for another item that holds no `stage.pending` +- **THEN** the job does not touch the branch or its pull request diff --git a/openspec/changes/4051-remove-the-orphan-draft-that-a/tasks.md b/openspec/changes/4051-remove-the-orphan-draft-that-a/tasks.md new file mode 100644 index 000000000..5410efe05 --- /dev/null +++ b/openspec/changes/4051-remove-the-orphan-draft-that-a/tasks.md @@ -0,0 +1,40 @@ +# Tasks + +## 1. Remove the orphan draft by the intent mark + +Kind: behavior. Reads: `src/devops_cli/roadmap/stage/stage.py`, `src/devops_cli/roadmap/stage/stage_rejected.py`, `src/devops_cli/roadmap/stores/store.py`, `src/devops_cli/roadmap/request_plan.py`, `tests/roadmap/stage/test_roadmap_stage_orphan.py`. Changes: `src/devops_cli/roadmap/stage/stage.py`, `src/devops_cli/roadmap/stores/store.py`, `src/devops_cli/roadmap/request_plan.py`, `src/devops_cli/config/constants.py`, `src/devops_cli/lang/en/messages.py`, `tests/roadmap/stage/test_roadmap_stage_orphan.py`. + +- [ ] 1.1 In `tests/roadmap/stage/test_roadmap_stage_orphan.py`, write the tests of the criteria: `marks_write_fails`, `killed_after_push`, `retitle_between`, `foreign_commit`, `other_branch_kept`, `young_mark` and `request_count`, the guard tests (a recorded pull request, a closed item, a branch that is gone) and the dry run test. Confirm that they fail on `origin/main` + - Verification: `$ uv run devops ci fail-to-pass --base origin/main` +- [ ] 1.2 Add the key `PENDING = "stage.pending"` to `StageRecordKey`, and the constant `CONST_ROADMAP_STAGE_ORPHAN_AGE_SECONDS` of 3600. In `_stage_one`, write `stage.pending` with one `set_marks` before the push with the `now` of the `StageWriter`, and clear it in the marks write after the draft + - Verification: `$ uv run devops ci test tests/roadmap/stage/test_roadmap_stage_orphan.py` +- [ ] 1.3 Add the pass `_orphans` next to `_reconcile`, call it in `plan_stage` with `now`, and add its results to `removals` and `kept`. A bad record gives no action. In `StageWriter.begin`, find the draft of an orphan removal by its head in one read of `store.open_pulls()` for the round, and close it in `_remove_staged` + - Verification: `$ uv run devops ci test tests/roadmap/stage/test_roadmap_stage_orphan.py` +- [ ] 1.4 Add the write of the mark before the push and the read of the open pull requests to `stage_requests` in `src/devops_cli/roadmap/request_plan.py`, with their texts in `src/devops_cli/lang/en/messages.py` + - Verification: `$ uv run devops ci test tests/roadmap/test_roadmap_dry_runs.py` +- [ ] 1.5 Run the tests of the stage job and the key-walk test of the example ConfigMap + - Verification: `$ uv run devops ci test tests/roadmap/stage` + +## 2. Fix the documents + +Kind: other. Reads: `docs/ROADMAP_WORKFLOW.md`, `docs/roadmap/stage.md`. Changes: `docs/ROADMAP_WORKFLOW.md`, `docs/roadmap/stage.md`, `src/devops_cli/roadmap/stores/store.py`. + +- [ ] 2.1 Add `stage.pending` to the table of keys in `docs/ROADMAP_WORKFLOW.md`, describe the orphan pass in `docs/roadmap/stage.md`, and update the docstring of `StageRecordKey` + - Verification: `$ uv run devops ci docs` +- [ ] 2.2 Check the change and the specs + - Verification: `$ uv run devops openspec check` + +## Criteria + +- Group 1: A test fails the marks write after the draft and runs the next round. It finds the draft closed, the branch deleted and the item planned again. +- Group 1: A kill between the push and the draft leaves a branch with no draft. The next round deletes the branch and clears the record. +- Group 1: A retitle between the failure and the next round changes the slug, and the mark still finds the orphan. +- Group 1: A branch with a commit of a person is kept and reported, not deleted. +- Group 1: The orphan of the job is removed, and a pushed branch of an agent for another Ready item with no mark is not touched. +- Group 1: A mark younger than the age limit is left alone. +- Group 1: A staged item adds 2 requests for the mark, a round with an orphan to remove adds 1 read of the open pull requests, and a round with a kept branch adds none. + +## Workflow follow-up + +- Pending a person: confirm the intent-mark approach of the proposal. +- Pending a person: review the diff in tuicr (tuicr -r ..HEAD). diff --git a/src/devops_cli/roadmap/stores/store.py b/src/devops_cli/roadmap/stores/store.py index ae71aa0e1..45e059bcd 100644 --- a/src/devops_cli/roadmap/stores/store.py +++ b/src/devops_cli/roadmap/stores/store.py @@ -235,6 +235,7 @@ class StageRecordKey(StrEnum): FAILED_ERROR = "stage.failed_error" TEXT_HASH = "stage.text_hash" REJECTED = "stage.rejected" + PENDING = "stage.pending" class ImplementRecordKey(StrEnum): diff --git a/tests/roadmap/stage/test_roadmap_stage_orphan.py b/tests/roadmap/stage/test_roadmap_stage_orphan.py new file mode 100644 index 000000000..ba7b5e33d --- /dev/null +++ b/tests/roadmap/stage/test_roadmap_stage_orphan.py @@ -0,0 +1,460 @@ +"""`devops roadmap stage`: the orphan draft that a failed marks write leaves (#4051). + +The job writes the intent mark `stage.pending` (the branch name and the time) before the push. A +round removes the draft and the branch of an open item that has the mark and no `stage.pr`, when +the mark is at least one hour old and only the identity of the clone wrote to the branch. A pushed +branch with no mark belongs to a person or to an agent: the job never touches it. +""" + +from __future__ import annotations + +import json +from datetime import UTC, datetime, timedelta +from typing import TYPE_CHECKING +from unittest.mock import patch + +import pytest + +from devops_cli.lang import MESSAGES +from devops_cli.roadmap.request_plan import StoreRequests, stage_requests +from devops_cli.roadmap.stage import stage as stage_module +from devops_cli.roadmap.stage.stage import StagePlan, parse_filters, plan_stage +from devops_cli.roadmap.stores.store import CloseReason, ItemField, StageRecordKey +from tests.support.builders import make_stage_proposal +from tests.support.fakes.github import API, github_json +from tests.support.fakes.roadmap_store import InMemoryRoadmapStore +from tests.support.roadmap_stage import ( + STAGE_NAME, + STAGE_REPO, + VALID_REPORT, + StageRepo, + apply_stage, + seed_stageable, + validate_args, +) + +if TYPE_CHECKING: + from unittest.mock import MagicMock + + from tests.support.fakes.github import GitHubApiFake + from tests.support.fakes.model import ModelFake + from tests.support.fakes.openspec import OpenSpecFake + +pytestmark = pytest.mark.usefixtures("keyring_store") + +NOW = datetime(2026, 11, 4, 12, 0, tzinfo=UTC) +LATER = NOW + timedelta(hours=2) +BRANCH = "fix/1-the-job-drops-a-comment" +OTHER_BRANCH = "fix/2-the-job-loses-a-label" +PULLS = f"/repos/{STAGE_REPO}/pulls" +OPENED = github_json({"number": 5, "html_url": "https://example.com/pull/5"}, status=201) +MARK_AGE = timedelta(hours=1) + + +class Killed(BaseException): + """The death of the worker: no handler of the job catches it.""" + + +def _mark(branch: str, time: datetime) -> str: + """The value of `stage.pending`: the branch name and the time, as JSON.""" + return json.dumps({"branch": branch, "time": time.isoformat()}) + + +def _pushed(repo: StageRepo) -> list[str]: + return repo.origin_git("branch", "--format=%(refname:short)").split() + + +def _record(store: InMemoryRoadmapStore, number: int = 1) -> dict[str, str | None]: + item = store.item(number) + assert item is not None + return { + key.value: value + for key, value in item.job_record.items() + if "." in key.value and value is not None + } + + +def _closing(api: GitHubApiFake, pull: int) -> str: + """Answer the close of draft `pull` and return the request line that makes it.""" + api.route("PATCH", f"{PULLS}/{pull}", github_json({"number": pull})) + return f"PATCH {API}{PULLS}/{pull}" + + +def _orphan( + store: InMemoryRoadmapStore, + repo: StageRepo, + api: GitHubApiFake, + *, + title: str = "fix(roadmap): the job drops a comment", + branch: str = BRANCH, + marked: datetime = NOW - timedelta(hours=3), + author: str = STAGE_NAME, + pushed: bool = True, + drafted: bool = True, + recorded_pull: str | None = None, +) -> tuple[int, str]: + """An open Ready item with the intent mark, the pushed branch and its draft. It has no + `type/bug` label, so a round with the filter `type=bug` selects no item and only the pass over + the orphans runs. Returns the item number and the request line that closes its draft.""" + number = store.seed_issue( + title, fields={ItemField.STATUS: "Ready", ItemField.PRIORITY: "P1-High"} + ) + item = store.item(number) + assert item is not None + marks = {StageRecordKey.PENDING: _mark(branch, marked)} + if recorded_pull is not None: + marks[StageRecordKey.PR] = recorded_pull + store.set_marks(item, marks) + if pushed: + repo.push_branch(branch, author=author) + close = "" + if drafted: + pull = store.seed_pull(f"plan {number}", head_ref=branch, draft=True) + close = _closing(api, pull) + return number, close + + +def _plan(store: InMemoryRoadmapStore, repo: StageRepo, *, now: datetime = LATER) -> StagePlan: + return plan_stage( + store, + repo=STAGE_REPO, + source=repo.clone, + filters=parse_filters(("status=ready", "type=bug")), + item_number=None, + now=now, + ) + + +def _round( + store: InMemoryRoadmapStore, repo: StageRepo, *, now: datetime = LATER +) -> tuple[StagePlan, tuple[int, ...], MagicMock]: + """One round that selects no item: its plan, the numbers it removed, and the spy on the read + of the open pull requests (the calls of the plan and of the writes).""" + with patch.object(store, "open_pulls", wraps=store.open_pulls) as reads: + plan = _plan(store, repo, now=now) + applied = apply_stage(store, plan, source=repo.clone, now=now) + return plan, applied.removed, reads + + +@pytest.fixture +def store() -> InMemoryRoadmapStore: + return InMemoryRoadmapStore(repo=STAGE_REPO) + + +@pytest.fixture +def staging( + store: InMemoryRoadmapStore, github_api: GitHubApiFake, openspec_fake: OpenSpecFake +) -> InMemoryRoadmapStore: + """A store with one Ready bug that a round stages, and the answers of GitHub and OpenSpec.""" + seed_stageable(store) + store.seed_file(".github/labels.yml", '- name: "type/docs"\n color: "0075ca"\n') + github_api.route("POST", PULLS, OPENED) + openspec_fake.script_sequence(validate_args("1-the-job-drops-a-comment"), (0, VALID_REPORT)) + return store + + +def _stage_round( + store: InMemoryRoadmapStore, repo: StageRepo, *, now: datetime +) -> tuple[StagePlan, tuple[int, ...], tuple[int, ...]]: + """One round with the default filter: the plan, the items it staged and the items it removed.""" + plan = plan_stage(store, repo=STAGE_REPO, source=repo.clone, filters=parse_filters(()), now=now) + applied = apply_stage(store, plan, source=repo.clone, now=now) + return plan, applied.staged, applied.removed + + +def test_a_round_after_the_marks_write_fails_removes_the_orphan_and_plans_the_item_again( + staging: InMemoryRoadmapStore, + stage_repo: StageRepo, + model_fake: ModelFake, + github_api: GitHubApiFake, +) -> None: + model_fake.answer(make_stage_proposal()) + # The first write of marks is the intent mark, the second one is the record after the draft. + staging.fail_write(write="set_marks", at=2) + + _, staged, _ = _stage_round(staging, stage_repo, now=NOW) + + assert staged == () + assert github_api.requested == [f"POST {API}{PULLS}"] + assert _pushed(stage_repo) == [BRANCH, "main"] + record = _record(staging) + assert StageRecordKey.PR.value not in record + mark = json.loads(record[StageRecordKey.PENDING.value] or "") + assert (mark["branch"], datetime.fromisoformat(mark["time"])) == (BRANCH, NOW) + + pull = staging.seed_pull("plan 1", head_ref=BRANCH, draft=True) + closing = _closing(github_api, pull) + github_api.requested.clear() + + plan, staged, removed = _stage_round(staging, stage_repo, now=LATER) + + assert [(r.number, r.branch, r.gone) for r in plan.removals] == [(1, BRANCH, False)] + assert (staged, removed) == ((), (1,)) + assert github_api.requested == [closing] + assert json.loads(github_api.sent[-1].content) == {"state": "closed"} + assert _pushed(stage_repo) == ["main"] + assert _record(staging) == {} + + github_api.requested.clear() + + _, staged, removed = _stage_round(staging, stage_repo, now=LATER) + + assert (staged, removed) == ((1,), ()) + assert github_api.requested == [f"POST {API}{PULLS}"] + assert _pushed(stage_repo) == [BRANCH, "main"] + record = _record(staging) + assert (record["stage.branch"], record["stage.pr"]) == (BRANCH, "5") + assert StageRecordKey.PENDING.value not in record + + +def test_a_worker_killed_after_the_push_leaves_a_branch_that_the_next_round_deletes_killed_after_push( + staging: InMemoryRoadmapStore, + stage_repo: StageRepo, + model_fake: ModelFake, + github_api: GitHubApiFake, + monkeypatch: pytest.MonkeyPatch, +) -> None: + model_fake.answer(make_stage_proposal()) + + def killed(*_: object) -> None: + raise Killed + + monkeypatch.setattr(stage_module, "_open_pull", killed) + plan = plan_stage( + staging, repo=STAGE_REPO, source=stage_repo.clone, filters=parse_filters(()), now=NOW + ) + with pytest.raises(Killed) as death: + apply_stage(staging, plan, source=stage_repo.clone, now=NOW) + assert death.type is Killed + monkeypatch.undo() + + assert _pushed(stage_repo) == [BRANCH, "main"] + assert github_api.requested == [] + record = _record(staging) + assert json.loads(record[StageRecordKey.PENDING.value] or "")["branch"] == BRANCH + assert StageRecordKey.PR.value not in record + + plan, _, removed = _stage_round(staging, stage_repo, now=LATER) + + assert [(r.number, r.branch, r.gone) for r in plan.removals] == [(1, BRANCH, False)] + assert removed == (1,) + assert github_api.requested == [] + assert _pushed(stage_repo) == ["main"] + assert _record(staging) == {} + + +def test_a_retitle_between_the_failure_and_the_next_round_leaves_the_orphan_found_by_its_mark( + store: InMemoryRoadmapStore, stage_repo: StageRepo, github_api: GitHubApiFake +) -> None: + number, closing = _orphan(store, stage_repo, github_api) + store.set_title(number, "fix(roadmap): the job loses a label") + + plan, removed, _ = _round(store, stage_repo) + + assert [(r.number, r.branch) for r in plan.removals] == [(1, BRANCH)] + assert removed == (1,) + assert github_api.requested == [closing] + assert _pushed(stage_repo) == ["main"] + assert _record(store) == {} + + +def test_a_branch_with_a_foreign_commit_is_kept_and_reported( + store: InMemoryRoadmapStore, stage_repo: StageRepo, github_api: GitHubApiFake +) -> None: + _orphan(store, stage_repo, github_api) + stage_repo.push_branch(BRANCH, author="Person") + + plan, removed, reads = _round(store, stage_repo) + + assert plan.removals == () + assert [(k.number, k.branch) for k in plan.kept] == [(1, BRANCH)] + assert "commit(s) by another author" in plan.kept[0].reason + assert removed == () + assert BRANCH in _pushed(stage_repo) + assert github_api.requested == [] + assert reads.call_count == 0 + assert StageRecordKey.PENDING.value in _record(store) + + +def test_the_orphan_of_the_job_goes_and_the_other_branch_of_an_agent_is_kept_other_branch_kept( + store: InMemoryRoadmapStore, stage_repo: StageRepo, github_api: GitHubApiFake +) -> None: + _, closing = _orphan(store, stage_repo, github_api) + agent = store.seed_issue( + "fix(roadmap): the job loses a label", + fields={ItemField.STATUS: "Ready", ItemField.PRIORITY: "P1-High"}, + ) + stage_repo.push_branch(OTHER_BRANCH, author="Agent") + agent_pull = store.seed_pull("agent work", head_ref=OTHER_BRANCH, draft=True) + _closing(github_api, agent_pull) + + plan, removed, _ = _round(store, stage_repo) + + assert [(r.number, r.branch) for r in plan.removals] == [(1, BRANCH)] + assert (plan.kept, removed) == ((), (1,)) + assert github_api.requested == [closing] + assert _pushed(stage_repo) == [OTHER_BRANCH, "main"] + assert _record(store, agent) == {} + + +@pytest.mark.parametrize( + "age", [timedelta(minutes=-5), timedelta(0), MARK_AGE - timedelta(seconds=1)] +) +def test_a_mark_younger_than_the_age_limit_is_left_alone_and_costs_no_request( + store: InMemoryRoadmapStore, stage_repo: StageRepo, github_api: GitHubApiFake, age: timedelta +) -> None: + _orphan(store, stage_repo, github_api, marked=LATER - age) + + plan, removed, reads = _round(store, stage_repo) + + assert (plan.removals, plan.kept, removed) == ((), (), ()) + assert github_api.requested == [] + assert reads.call_count == 0 + assert BRANCH in _pushed(stage_repo) + assert StageRecordKey.PENDING.value in _record(store) + + +def test_a_young_mark_is_left_alone_until_it_reaches_the_age_limit_young_mark( + store: InMemoryRoadmapStore, stage_repo: StageRepo, github_api: GitHubApiFake +) -> None: + _, closing = _orphan(store, stage_repo, github_api, marked=NOW) + + plan, removed, _ = _round(store, stage_repo, now=NOW + MARK_AGE - timedelta(seconds=1)) + + assert (plan.removals, plan.kept, removed) == ((), (), ()) + assert github_api.requested == [] + assert BRANCH in _pushed(stage_repo) + + plan, removed, _ = _round(store, stage_repo, now=NOW + MARK_AGE) + + assert [r.number for r in plan.removals] == [1] + assert removed == (1,) + assert github_api.requested == [closing] + assert _pushed(stage_repo) == ["main"] + + +def test_a_mark_of_exactly_the_age_limit_is_judged( + store: InMemoryRoadmapStore, stage_repo: StageRepo, github_api: GitHubApiFake +) -> None: + _, closing = _orphan(store, stage_repo, github_api, marked=LATER - MARK_AGE) + + plan, removed, _ = _round(store, stage_repo) + + assert [r.number for r in plan.removals] == [1] + assert removed == (1,) + assert github_api.requested == [closing] + + +def test_an_orphan_whose_branch_is_gone_loses_its_record_and_costs_no_request( + store: InMemoryRoadmapStore, stage_repo: StageRepo, github_api: GitHubApiFake +) -> None: + _orphan(store, stage_repo, github_api, pushed=False, drafted=False) + + plan, removed, reads = _round(store, stage_repo) + + assert [(r.number, r.gone) for r in plan.removals] == [(1, True)] + assert removed == (1,) + assert github_api.requested == [] + assert reads.call_count == 0 + assert _record(store) == {} + + +def test_an_item_with_a_recorded_pull_request_is_no_orphan( + store: InMemoryRoadmapStore, stage_repo: StageRepo, github_api: GitHubApiFake +) -> None: + _orphan(store, stage_repo, github_api, recorded_pull="9") + github_api.route( + "GET", + f"{PULLS}/9", + github_json({"state": "open", "draft": True, "merged": False}), + ) + + plan, removed, reads = _round(store, stage_repo) + + assert (plan.removals, plan.kept, removed) == ((), (), ()) + assert github_api.requested == [f"GET {API}{PULLS}/9"] + assert reads.call_count == 0 + assert BRANCH in _pushed(stage_repo) + + +def test_a_closed_item_with_the_mark_is_not_an_orphan( + store: InMemoryRoadmapStore, stage_repo: StageRepo, github_api: GitHubApiFake +) -> None: + number, _ = _orphan(store, stage_repo, github_api) + store.close_by_hand(number, CloseReason.COMPLETED) + + plan, removed, _ = _round(store, stage_repo) + + assert (plan.removals, plan.kept, removed) == ((), (), ()) + assert BRANCH in _pushed(stage_repo) + + +def test_a_staged_item_costs_one_more_write_of_marks_for_the_intent_request_count( + staging: InMemoryRoadmapStore, stage_repo: StageRepo, model_fake: ModelFake +) -> None: + model_fake.answer(make_stage_proposal()) + + with patch.object(staging, "open_pulls", wraps=staging.open_pulls) as reads: + _, staged, _ = _stage_round(staging, stage_repo, now=NOW) + + assert staged == (1,) + assert staging.writes.count("set_marks") == 2 + assert StageRecordKey.PENDING.value not in _record(staging) + assert reads.call_count == 0 + + +def test_a_round_with_two_orphans_reads_the_open_pull_requests_once_request_count( + store: InMemoryRoadmapStore, stage_repo: StageRepo, github_api: GitHubApiFake +) -> None: + _, first = _orphan(store, stage_repo, github_api) + _, second = _orphan( + store, + stage_repo, + github_api, + title="fix(roadmap): the job loses a label", + branch=OTHER_BRANCH, + ) + + plan, removed, reads = _round(store, stage_repo) + + assert [r.number for r in plan.removals] == [1, 2] + assert removed == (1, 2) + assert reads.call_count == 1 + assert github_api.requested == [first, second] + + +def test_a_round_with_an_orphan_and_no_draft_still_reads_the_open_pull_requests_once_request_count( + store: InMemoryRoadmapStore, stage_repo: StageRepo, github_api: GitHubApiFake +) -> None: + _orphan(store, stage_repo, github_api, drafted=False) + + _, removed, reads = _round(store, stage_repo) + + assert removed == (1,) + assert reads.call_count == 1 + assert github_api.requested == [] + + +def test_a_round_without_an_orphan_reads_no_open_pull_requests_request_count( + store: InMemoryRoadmapStore, stage_repo: StageRepo +) -> None: + store.seed_issue("fix(roadmap): the job drops a comment") + stage_repo.push_branch(BRANCH, author="Person") + + plan, removed, reads = _round(store, stage_repo) + + assert (plan.removals, plan.kept, removed) == ((), (), ()) + assert reads.call_count == 0 + assert BRANCH in _pushed(stage_repo) + + +def test_the_dry_run_lists_the_write_of_the_intent_mark_before_the_push() -> None: + _, writes = stage_requests(STAGE_REPO) + + texts = MESSAGES.roadmap + record = StoreRequests(STAGE_REPO).set_marks(texts.plan_placeholders["item"])[-1] + marks = [i for i, w in enumerate(writes) if (w.target, w.argv) == (record.target, record.argv)] + push = next(i for i, w in enumerate(writes) if "push" in w.argv and "-u" in w.argv) + worktree = next(i for i, w in enumerate(writes) if "worktree" in w.argv and "add" in w.argv) + + assert [i for i in marks if worktree < i < push] != [] From 053264c7581a4f7f4a5a5f8cc9d7e1394b82ab60 Mon Sep 17 00:00:00 2001 From: Daniel Petty Date: Sun, 8 Nov 2026 05:22:14 +0000 Subject: [PATCH 2/4] fix(roadmap): remove the orphan draft that a failed marks write leaves Plans #4051. --- docs/ROADMAP_WORKFLOW.md | 1 + docs/roadmap/stage.md | 12 ++++ .../tasks.md | 14 ++-- src/devops_cli/config/constants.py | 3 + src/devops_cli/lang/en/messages.py | 1 + src/devops_cli/roadmap/request_plan.py | 2 + src/devops_cli/roadmap/stage/stage.py | 67 ++++++++++++++++--- src/devops_cli/roadmap/stage/stage_pending.py | 61 +++++++++++++++++ src/devops_cli/roadmap/stores/store.py | 5 +- .../stage/test_roadmap_stage_item_writes.py | 3 +- .../stage/test_roadmap_stage_orphan.py | 20 +++--- 11 files changed, 159 insertions(+), 30 deletions(-) create mode 100644 src/devops_cli/roadmap/stage/stage_pending.py diff --git a/docs/ROADMAP_WORKFLOW.md b/docs/ROADMAP_WORKFLOW.md index 49d958fa1..f82bdab43 100644 --- a/docs/ROADMAP_WORKFLOW.md +++ b/docs/ROADMAP_WORKFLOW.md @@ -122,6 +122,7 @@ These are all the keys of the job record, by the job that writes them: | Stage | `stage.groups`, `stage.needs_split`, `stage.file_cap` | The task groups with their size, the group over the budget, and the cap of that measure | | Stage | `stage.ready` | The plan pull request that the Service has seen leave the draft state | | Stage | `stage.needs_fix` | The plan pull request that the Service told about the label `plan/needs-fix` | +| Stage | `stage.pending` | The intent mark of the stage job, as JSON with the branch name and the time. The job writes it before the push and clears it with the record after the draft. An open Item with the mark, a pushed branch and no `stage.pr` is an orphan: a round at least 1 hour later closes its draft, deletes its branch and clears the record, unless another author wrote to the branch | | Stage | `stage.text_hash` | The hash of the text of the Item that the plan came from | | Stage | `stage.rejected` | A plan that a person rejected, as JSON. It holds the pull request, the time of the close, the files that the reason names, and the text hash. The job leaves the Item out until the text or one of those files changes. A staged plan clears it | | Stage | `stage.failed_at`, `stage.failed_error` | When the last plan or write of the Item failed and the class of its error. The job leaves the Item out for `roadmap.stage.retry_hours` hours; a staged Item or an Item stopped on the budget clears both | diff --git a/docs/roadmap/stage.md b/docs/roadmap/stage.md index e48ac029c..255a96c3b 100644 --- a/docs/roadmap/stage.md +++ b/docs/roadmap/stage.md @@ -78,6 +78,18 @@ The reply of the planner has no key to say that the premise of the issue is fals Each run also removes the draft and the branch of a closed item, when only the identity of the session wrote to them. +## The orphan draft of a failed write + +The job writes the intent mark `stage.pending` before it pushes the branch of an item. The mark is JSON with the branch name and the time. The write of the record after the draft clears it. When that write fails, or the worker dies between the push and the record, the item keeps the mark and has a pushed branch and a draft but no `stage.pr`. This is an orphan. + +In each run the job judges an open item that holds the mark and no `stage.pr`. A mark younger than 1 hour is left alone, because another run can be in the middle of its publish. For an older mark the job does these things: + +- When the branch of the mark is gone, it clears the record of the item. +- When only the identity of the session wrote to the branch, it closes the draft pull request that has this branch as its head, deletes the branch and clears the record. The job finds the draft with one read of the open pull requests for the round. It makes this read only when it has a pushed branch to remove. The next round plans the item again. +- When a commit past the default branch has another author, it keeps the branch, and the plan lists it. + +A pushed branch with no mark is never an orphan, because a person or another agent can own it. A retitle between the failure and the next round does not matter, because the job uses the branch of the mark and not the slug of the title. A closed item that holds the mark is not judged. A bad value of the mark counts as no mark. + ## A plan that a person rejected A person rejects a plan when they close its pull request with no merge. The reason stays on the closed pull request, so the stage job copies it to the issue. In each run the job reads the plan pull request of each staged item. A staged item is an open New or Ready item that holds `stage.pr` and has no plan on the default branch. With `--item` the job reads the pull request of that item only. For a pull request that is closed and not merged, the job does these things in order: diff --git a/openspec/changes/4051-remove-the-orphan-draft-that-a/tasks.md b/openspec/changes/4051-remove-the-orphan-draft-that-a/tasks.md index 5410efe05..b419ad78b 100644 --- a/openspec/changes/4051-remove-the-orphan-draft-that-a/tasks.md +++ b/openspec/changes/4051-remove-the-orphan-draft-that-a/tasks.md @@ -4,24 +4,24 @@ Kind: behavior. Reads: `src/devops_cli/roadmap/stage/stage.py`, `src/devops_cli/roadmap/stage/stage_rejected.py`, `src/devops_cli/roadmap/stores/store.py`, `src/devops_cli/roadmap/request_plan.py`, `tests/roadmap/stage/test_roadmap_stage_orphan.py`. Changes: `src/devops_cli/roadmap/stage/stage.py`, `src/devops_cli/roadmap/stores/store.py`, `src/devops_cli/roadmap/request_plan.py`, `src/devops_cli/config/constants.py`, `src/devops_cli/lang/en/messages.py`, `tests/roadmap/stage/test_roadmap_stage_orphan.py`. -- [ ] 1.1 In `tests/roadmap/stage/test_roadmap_stage_orphan.py`, write the tests of the criteria: `marks_write_fails`, `killed_after_push`, `retitle_between`, `foreign_commit`, `other_branch_kept`, `young_mark` and `request_count`, the guard tests (a recorded pull request, a closed item, a branch that is gone) and the dry run test. Confirm that they fail on `origin/main` +- [x] 1.1 In `tests/roadmap/stage/test_roadmap_stage_orphan.py`, write the tests of the criteria: `marks_write_fails`, `killed_after_push`, `retitle_between`, `foreign_commit`, `other_branch_kept`, `young_mark` and `request_count`, the guard tests (a recorded pull request, a closed item, a branch that is gone) and the dry run test. Confirm that they fail on `origin/main` - Verification: `$ uv run devops ci fail-to-pass --base origin/main` -- [ ] 1.2 Add the key `PENDING = "stage.pending"` to `StageRecordKey`, and the constant `CONST_ROADMAP_STAGE_ORPHAN_AGE_SECONDS` of 3600. In `_stage_one`, write `stage.pending` with one `set_marks` before the push with the `now` of the `StageWriter`, and clear it in the marks write after the draft +- [x] 1.2 Add the key `PENDING = "stage.pending"` to `StageRecordKey`, and the constant `CONST_ROADMAP_STAGE_ORPHAN_AGE_SECONDS` of 3600. In `_stage_one`, write `stage.pending` with one `set_marks` before the push with the `now` of the `StageWriter`, and clear it in the marks write after the draft - Verification: `$ uv run devops ci test tests/roadmap/stage/test_roadmap_stage_orphan.py` -- [ ] 1.3 Add the pass `_orphans` next to `_reconcile`, call it in `plan_stage` with `now`, and add its results to `removals` and `kept`. A bad record gives no action. In `StageWriter.begin`, find the draft of an orphan removal by its head in one read of `store.open_pulls()` for the round, and close it in `_remove_staged` +- [x] 1.3 Add the pass `_orphans` next to `_reconcile`, call it in `plan_stage` with `now`, and add its results to `removals` and `kept`. A bad record gives no action. In `StageWriter.begin`, find the draft of an orphan removal by its head in one read of `store.open_pulls()` for the round, and close it in `_remove_staged` - Verification: `$ uv run devops ci test tests/roadmap/stage/test_roadmap_stage_orphan.py` -- [ ] 1.4 Add the write of the mark before the push and the read of the open pull requests to `stage_requests` in `src/devops_cli/roadmap/request_plan.py`, with their texts in `src/devops_cli/lang/en/messages.py` +- [x] 1.4 Add the write of the mark before the push and the read of the open pull requests to `stage_requests` in `src/devops_cli/roadmap/request_plan.py`, with their texts in `src/devops_cli/lang/en/messages.py` - Verification: `$ uv run devops ci test tests/roadmap/test_roadmap_dry_runs.py` -- [ ] 1.5 Run the tests of the stage job and the key-walk test of the example ConfigMap +- [x] 1.5 Run the tests of the stage job and the key-walk test of the example ConfigMap - Verification: `$ uv run devops ci test tests/roadmap/stage` ## 2. Fix the documents Kind: other. Reads: `docs/ROADMAP_WORKFLOW.md`, `docs/roadmap/stage.md`. Changes: `docs/ROADMAP_WORKFLOW.md`, `docs/roadmap/stage.md`, `src/devops_cli/roadmap/stores/store.py`. -- [ ] 2.1 Add `stage.pending` to the table of keys in `docs/ROADMAP_WORKFLOW.md`, describe the orphan pass in `docs/roadmap/stage.md`, and update the docstring of `StageRecordKey` +- [x] 2.1 Add `stage.pending` to the table of keys in `docs/ROADMAP_WORKFLOW.md`, describe the orphan pass in `docs/roadmap/stage.md`, and update the docstring of `StageRecordKey` - Verification: `$ uv run devops ci docs` -- [ ] 2.2 Check the change and the specs +- [x] 2.2 Check the change and the specs - Verification: `$ uv run devops openspec check` ## Criteria diff --git a/src/devops_cli/config/constants.py b/src/devops_cli/config/constants.py index d65e9634a..0fab3e492 100644 --- a/src/devops_cli/config/constants.py +++ b/src/devops_cli/config/constants.py @@ -3526,6 +3526,9 @@ class WebhookExposure(StrEnum): CONST_ROADMAP_STAGE_COMMENT_MARKER, CONST_ROADMAP_STAGE_REJECTED_MARKER, ) +# The age at which the stage job judges its intent mark `stage.pending` (#4051): a mark younger +# than this is left alone, since another run can be in the middle of its publish. +CONST_ROADMAP_STAGE_ORPHAN_AGE_SECONDS: Final[int] = 3600 # The comment marker of `devops roadmap retitle` (#3486). CONST_ROADMAP_RETITLE_COMMENT_MARKER: Final[str] = "" # The marker of every roadmap job's comment: a comment that holds none of them is a person's. diff --git a/src/devops_cli/lang/en/messages.py b/src/devops_cli/lang/en/messages.py index 905cd9307..68b20ddf0 100644 --- a/src/devops_cli/lang/en/messages.py +++ b/src/devops_cli/lang/en/messages.py @@ -3504,6 +3504,7 @@ class RoadmapMessages: "model_correction": "the change fails its checks", "validate": "the openspec binary is on PATH", "delete_failed": "GitHub refuses the pull request", + "orphan_pulls": "the round removes the pushed branch of an orphan draft", "close_pull": "only the identity of the session wrote to the branch", "delete_branch": "only the identity of the session wrote to the branch", "plan_state": "the record of an open New or Ready item holds a staged pull request", diff --git a/src/devops_cli/roadmap/request_plan.py b/src/devops_cli/roadmap/request_plan.py index c30957b5b..df91a8e3c 100644 --- a/src/devops_cli/roadmap/request_plan.py +++ b/src/devops_cli/roadmap/request_plan.py @@ -653,6 +653,7 @@ def other(method: str, key: str, *argv: str, when: str = "") -> PlannedRequest: repeat=rep["open_plan"], ), git("fetch_branch", clone, "fetch", "-q", "origin", branch, repeat=each_closed), + *within(store.open_pulls(), cond["orphan_pulls"]), *within(store.comments_on(_p("item")), cond["rejected_reason"], repeat=each_item), PlannedRequest("model", req["model_change"], repeat=each_item), PlannedRequest( @@ -708,6 +709,7 @@ def other(method: str, key: str, *argv: str, when: str = "") -> PlannedRequest: "", repeat=each_item, ), + *within(store.set_marks(_p("item")), repeat=each_item), git("commit", tree, "add", "--", f"openspec/changes/{change}", repeat=each_item), git("commit", tree, "commit", "-q", "-m", _p("title"), repeat=each_item), git("push", tree, "push", "-q", "-u", "origin", branch, repeat=each_item), diff --git a/src/devops_cli/roadmap/stage/stage.py b/src/devops_cli/roadmap/stage/stage.py index 39b641a02..f21cb6f3d 100644 --- a/src/devops_cli/roadmap/stage/stage.py +++ b/src/devops_cli/roadmap/stage/stage.py @@ -187,6 +187,7 @@ session_pull_reads, ) from devops_cli.roadmap.stage.stage_paths import StagedTree +from devops_cli.roadmap.stage.stage_pending import PendingStage, orphan_mark from devops_cli.roadmap.stage.stage_probe import blocked_by, probe_premise from devops_cli.roadmap.stage.stage_registry import matches, registry_table from devops_cli.roadmap.stage.stage_rejected import ( @@ -755,12 +756,15 @@ class StageFailure: @dataclass(frozen=True) class Removal: """The staged branch of a closed item that only the session's identity wrote to: the job - closes its draft pull request and deletes it, or, when `gone`, only clears the record.""" + closes its draft pull request and deletes it, or, when `gone`, only clears the record. With + `orphan`, the item is open and holds the intent mark of the job and no pull request (#4051): + the writer finds the draft by its branch.""" item: Item branch: str pull: int | None gone: bool = False + orphan: bool = False @property def number(self) -> int: @@ -1323,6 +1327,30 @@ def _reconcile( return tuple(removals), tuple(kept) +def _orphans( + store: RoadmapStore, source: Path, remote_heads: str, now: datetime +) -> tuple[tuple[Removal, ...], tuple[KeptBranch, ...]]: + """Judge the orphan of each open item that holds the intent mark `stage.pending` and no + pull request, when the mark is old enough (#4051). The branch is the one the mark names. It is + removed when it is gone or every commit past the default branch is the clone identity's, else + kept. A pushed branch with no mark is never judged.""" + pushed = set(remote_branches(remote_heads)) + removals: list[Removal] = [] + kept: list[KeptBranch] = [] + for item in store.items(): + if (pending := orphan_mark(item, now)) is None: + continue + branch = pending.branch + if branch not in pushed: + removals.append(Removal(item, branch, None, gone=True, orphan=True)) + elif (foreign := _foreign_commits(source, branch)) == 0: + removals.append(Removal(item, branch, None, orphan=True)) + else: + reason = MESSAGES.roadmap.stage_kept_reason.format(count=foreign) + kept.append(KeptBranch(item.number, branch, reason)) + return tuple(removals), tuple(kept) + + class Rejections(NamedTuple): """The plans that a person closed with no merge, the staged branches that the job keeps for them, and the items whose plan pull request could not be read.""" @@ -1477,14 +1505,15 @@ def plan_stage( ) budget = step_tokens or settings.roadmap.stage.step_tokens removals, kept = _reconcile(store, checkout, heads) + orphans, orphans_kept = _orphans(store, checkout, heads, now or datetime.now(UTC)) unmatched = unmatched_items(store, filters) if why and item_number is None else () plan = StagePlan( repo, sha, budget, left_out=selection.left_out, - removals=removals, - kept=(*kept, *rejections.kept), + removals=(*removals, *orphans), + kept=(*kept, *orphans_kept, *rejections.kept), planned=planned, unmatched=unmatched, failed=tuple(_write_failure(f.item, f.error) for f in rejections.failures), @@ -1858,13 +1887,19 @@ def _stage_one( base: str, change: StagedChange, moved: Sequence[MovedPath], + now: datetime, ) -> None: """Make the branch, the commit, the push and the draft of one item, then record them and post the one comment. The branch starts at the staged commit `base` (#3301). The Status of the item stays as it is. `moved` is the named paths that the default branch changed after `base` - (#3309).""" + (#3309). The intent mark `stage.pending` goes first, before the push, so that a later round + finds the branch if the record write fails or the worker dies (#4051); the record clears it.""" target = change.target with _worktree(source, target, base) as path: + store.set_marks( + target.item, + {StageRecordKey.PENDING: PendingStage(branch=target.branch, time=now).record}, + ) sha = _commit_and_push(path, change) pull = _open_pull(repo, source, change) store.set_marks( @@ -1880,6 +1915,7 @@ def _stage_one( StageRecordKey.FAILED_AT: None, StageRecordKey.FAILED_ERROR: None, StageRecordKey.REJECTED: None, + StageRecordKey.PENDING: None, }, ) label_note = _label_pull(store, pull.number, change.title) @@ -2082,12 +2118,10 @@ def begin(self, plan: StagePlan, selected: Collection[int]) -> None: and the failures that `plan` holds. A rejection of a selected item waits for its plan.""" self._repo, self._sha = plan.repo, plan.sha self._rejections = {r.number: r for r in plan.rejections if r.number in selected} + drafts = cache(self._draft_heads) with self._recording(plan.failed): write_each( - [ - (r.item, partial(_remove_staged, self._store, plan.repo, self._source, r)) - for r in plan.removals - ], + [(r.item, partial(self._remove, plan.repo, r, drafts)) for r in plan.removals], self._removed, self._failed, ) @@ -2104,6 +2138,20 @@ def begin(self, plan: StagePlan, selected: Collection[int]) -> None: self._failed, ) + def _draft_heads(self) -> dict[str, int]: + """The number of the open pull request of each head branch: one read for the round.""" + drafts: dict[str, int] = {} + for pull in self._store.open_pulls(): + drafts.setdefault(pull.head_ref, pull.number) + return drafts + + def _remove(self, repo: str, removal: Removal, drafts: Callable[[], Mapping[str, int]]) -> None: + """Remove a staged branch. The draft of an orphan is not in the record, so it is found by + its head branch in the one read of the open pull requests (#4051).""" + if removal.orphan and not removal.gone: + removal = replace(removal, pull=drafts().get(removal.branch)) + _remove_staged(self._store, repo, self._source, removal) + def settle( self, target: StageTarget, result: StagedChange | OverBudget | Blocked | StageFailure ) -> None: @@ -2137,7 +2185,8 @@ def _settle( def _stage(self, change: StagedChange) -> None: moved = _moved_for(self._source, self._sha, change) - _stage_one(self._store, self._repo, self._source, self._sha, change, moved) + now = self._now or datetime.now(UTC) + _stage_one(self._store, self._repo, self._source, self._sha, change, moved, now) @contextmanager def _recording(self, planned: Iterable[StageFailure]) -> Iterator[None]: diff --git a/src/devops_cli/roadmap/stage/stage_pending.py b/src/devops_cli/roadmap/stage/stage_pending.py new file mode 100644 index 000000000..9a164b461 --- /dev/null +++ b/src/devops_cli/roadmap/stage/stage_pending.py @@ -0,0 +1,61 @@ +"""The intent mark of the stage job (#4051). + +The job writes `stage.pending` before it pushes the branch of an item, and the record after the +draft pull request clears it. When the record write fails, or the worker dies between the push and +the record, the mark stays with a pushed branch and no `stage.pr`: an orphan. Only this mark proves +that the job made the branch, so a pushed branch with no mark, which can belong to a person or to +an agent, is never an orphan. A mark younger than `CONST_ROADMAP_STAGE_ORPHAN_AGE_SECONDS` is left +alone, since another run can be in the middle of its publish. +""" + +from __future__ import annotations + +import logging +from datetime import datetime, timedelta + +from pydantic import AwareDatetime, BaseModel, ConfigDict + +from devops_cli.config.constants import CONST_ROADMAP_STAGE_ORPHAN_AGE_SECONDS +from devops_cli.roadmap.stores.store import GitHubState, Item, StageRecordKey + +logger = logging.getLogger(__name__) + + +class PendingStage(BaseModel): + """The record `stage.pending`: the branch the job is about to push and the time of the mark.""" + + model_config = ConfigDict(frozen=True, extra="forbid") + + branch: str + time: AwareDatetime + + @property + def record(self) -> str: + """The value of `stage.pending`.""" + return self.model_dump_json() + + +def recorded_pending(item: Item) -> PendingStage | None: + """The intent mark that the job record of `item` holds, or None when it holds none. A record + that is no mark is logged and read as none, so it never stops a round.""" + recorded = item.job_record.get(StageRecordKey.PENDING) + if not recorded: + return None + try: + return PendingStage.model_validate_json(recorded) + except ValueError: + logger.warning("The pending stage recorded on #%d is not an intent mark.", item.number) + return None + + +def orphan_mark(item: Item, now: datetime) -> PendingStage | None: + """The mark of `item` when the item can hold an orphan: it is open, its record holds no + pull request, and the mark is at least as old as the age limit at `now`.""" + if item.state is not GitHubState.OPEN or item.job_record.get(StageRecordKey.PR): + return None + pending = recorded_pending(item) + if pending is None: + return None + if now - pending.time < timedelta(seconds=CONST_ROADMAP_STAGE_ORPHAN_AGE_SECONDS): + return None + return pending diff --git a/src/devops_cli/roadmap/stores/store.py b/src/devops_cli/roadmap/stores/store.py index 45e059bcd..af93bc7b7 100644 --- a/src/devops_cli/roadmap/stores/store.py +++ b/src/devops_cli/roadmap/stores/store.py @@ -221,7 +221,10 @@ class StageRecordKey(StrEnum): or stops it on the budget. `stage.text_hash` is the hash of the text that the plan came from (#3503). When a person closes the plan pull request with no merge, the job clears every key and records the rejection in `stage.rejected` as JSON (#3503): the pull request, the hash of the - text that the plan came from, the time of the close and the files that the reason names.""" + text that the plan came from, the time of the close and the files that the reason names. The + job writes `stage.pending`, the branch name and the time as JSON, before it pushes the branch + and clears it in the record after the draft (#4051): an open item with the mark, a pushed + branch and no `stage.pr` is an orphan that a later round removes.""" BRANCH = "stage.branch" PR = "stage.pr" diff --git a/tests/roadmap/stage/test_roadmap_stage_item_writes.py b/tests/roadmap/stage/test_roadmap_stage_item_writes.py index ea5e2fe61..4d172ab6d 100644 --- a/tests/roadmap/stage/test_roadmap_stage_item_writes.py +++ b/tests/roadmap/stage/test_roadmap_stage_item_writes.py @@ -322,7 +322,8 @@ def plan(prompt: str) -> Any: ("comment", 2, "rejected"), ("field", 2), ("plan", 3), - ("marks", 3), + ("marks", 3), # the intent mark, before the push + ("marks", 3), # the record, after the draft ("comment", 3, "staged"), ] diff --git a/tests/roadmap/stage/test_roadmap_stage_orphan.py b/tests/roadmap/stage/test_roadmap_stage_orphan.py index ba7b5e33d..511afc625 100644 --- a/tests/roadmap/stage/test_roadmap_stage_orphan.py +++ b/tests/roadmap/stage/test_roadmap_stage_orphan.py @@ -184,7 +184,7 @@ def test_a_round_after_the_marks_write_fails_removes_the_orphan_and_plans_the_it pull = staging.seed_pull("plan 1", head_ref=BRANCH, draft=True) closing = _closing(github_api, pull) - github_api.requested.clear() + github_api.sent.clear() plan, staged, removed = _stage_round(staging, stage_repo, now=LATER) @@ -195,7 +195,7 @@ def test_a_round_after_the_marks_write_fails_removes_the_orphan_and_plans_the_it assert _pushed(stage_repo) == ["main"] assert _record(staging) == {} - github_api.requested.clear() + github_api.sent.clear() _, staged, removed = _stage_round(staging, stage_repo, now=LATER) @@ -212,21 +212,17 @@ def test_a_worker_killed_after_the_push_leaves_a_branch_that_the_next_round_dele stage_repo: StageRepo, model_fake: ModelFake, github_api: GitHubApiFake, - monkeypatch: pytest.MonkeyPatch, ) -> None: model_fake.answer(make_stage_proposal()) - - def killed(*_: object) -> None: - raise Killed - - monkeypatch.setattr(stage_module, "_open_pull", killed) plan = plan_stage( staging, repo=STAGE_REPO, source=stage_repo.clone, filters=parse_filters(()), now=NOW ) - with pytest.raises(Killed) as death: + with ( + patch.object(stage_module, "_open_pull", side_effect=Killed), + pytest.raises(Killed) as death, + ): apply_stage(staging, plan, source=stage_repo.clone, now=NOW) assert death.type is Killed - monkeypatch.undo() assert _pushed(stage_repo) == [BRANCH, "main"] assert github_api.requested == [] @@ -417,8 +413,8 @@ def test_a_round_with_two_orphans_reads_the_open_pull_requests_once_request_coun plan, removed, reads = _round(store, stage_repo) - assert [r.number for r in plan.removals] == [1, 2] - assert removed == (1, 2) + assert [r.number for r in plan.removals] == [1, 3] + assert removed == (1, 3) assert reads.call_count == 1 assert github_api.requested == [first, second] From ccc8cd2b35c6c19d95aaa3a6ad68faaea7a34bdc Mon Sep 17 00:00:00 2001 From: Daniel Petty Date: Sun, 8 Nov 2026 05:43:57 +0000 Subject: [PATCH 3/4] test(roadmap): class the orphan age limit of the stage job --- tests/roadmap/test_roadmap_limit_classes.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/roadmap/test_roadmap_limit_classes.py b/tests/roadmap/test_roadmap_limit_classes.py index ecbe8b9c2..d854da89f 100644 --- a/tests/roadmap/test_roadmap_limit_classes.py +++ b/tests/roadmap/test_roadmap_limit_classes.py @@ -200,6 +200,10 @@ "kept", "planner calls that repair one plan; it bounds the cost of a plan that the review returns", ), + "CONST_ROADMAP_STAGE_ORPHAN_AGE_SECONDS": ( + "kept", + "the age of a pending mark before the stage job judges it; a younger mark can belong to a run that still pushes", + ), "CONST_ROADMAP_VERIFY_RECALL_FLOOR": ( "other", "the recall that the verify eval must reach; a score bar, not a limit of a job", From 91b1e609b526d9bba1a45d8fb99d30ff181898ed Mon Sep 17 00:00:00 2001 From: Daniel Petty Date: Sun, 8 Nov 2026 05:44:26 +0000 Subject: [PATCH 4/4] chore(openspec): archive the change 4051-remove-the-orphan-draft-that-a --- .../.openspec.yaml | 0 .../design.md | 0 .../proposal.md | 0 .../specs/roadmap-stage/spec.md | 0 .../tasks.md | 0 openspec/specs/roadmap-stage/spec.md | 48 +++++++++++++++++++ 6 files changed, 48 insertions(+) rename openspec/changes/{4051-remove-the-orphan-draft-that-a => archive/2026-11-08-4051-remove-the-orphan-draft-that-a}/.openspec.yaml (100%) rename openspec/changes/{4051-remove-the-orphan-draft-that-a => archive/2026-11-08-4051-remove-the-orphan-draft-that-a}/design.md (100%) rename openspec/changes/{4051-remove-the-orphan-draft-that-a => archive/2026-11-08-4051-remove-the-orphan-draft-that-a}/proposal.md (100%) rename openspec/changes/{4051-remove-the-orphan-draft-that-a => archive/2026-11-08-4051-remove-the-orphan-draft-that-a}/specs/roadmap-stage/spec.md (100%) rename openspec/changes/{4051-remove-the-orphan-draft-that-a => archive/2026-11-08-4051-remove-the-orphan-draft-that-a}/tasks.md (100%) diff --git a/openspec/changes/4051-remove-the-orphan-draft-that-a/.openspec.yaml b/openspec/changes/archive/2026-11-08-4051-remove-the-orphan-draft-that-a/.openspec.yaml similarity index 100% rename from openspec/changes/4051-remove-the-orphan-draft-that-a/.openspec.yaml rename to openspec/changes/archive/2026-11-08-4051-remove-the-orphan-draft-that-a/.openspec.yaml diff --git a/openspec/changes/4051-remove-the-orphan-draft-that-a/design.md b/openspec/changes/archive/2026-11-08-4051-remove-the-orphan-draft-that-a/design.md similarity index 100% rename from openspec/changes/4051-remove-the-orphan-draft-that-a/design.md rename to openspec/changes/archive/2026-11-08-4051-remove-the-orphan-draft-that-a/design.md diff --git a/openspec/changes/4051-remove-the-orphan-draft-that-a/proposal.md b/openspec/changes/archive/2026-11-08-4051-remove-the-orphan-draft-that-a/proposal.md similarity index 100% rename from openspec/changes/4051-remove-the-orphan-draft-that-a/proposal.md rename to openspec/changes/archive/2026-11-08-4051-remove-the-orphan-draft-that-a/proposal.md diff --git a/openspec/changes/4051-remove-the-orphan-draft-that-a/specs/roadmap-stage/spec.md b/openspec/changes/archive/2026-11-08-4051-remove-the-orphan-draft-that-a/specs/roadmap-stage/spec.md similarity index 100% rename from openspec/changes/4051-remove-the-orphan-draft-that-a/specs/roadmap-stage/spec.md rename to openspec/changes/archive/2026-11-08-4051-remove-the-orphan-draft-that-a/specs/roadmap-stage/spec.md diff --git a/openspec/changes/4051-remove-the-orphan-draft-that-a/tasks.md b/openspec/changes/archive/2026-11-08-4051-remove-the-orphan-draft-that-a/tasks.md similarity index 100% rename from openspec/changes/4051-remove-the-orphan-draft-that-a/tasks.md rename to openspec/changes/archive/2026-11-08-4051-remove-the-orphan-draft-that-a/tasks.md diff --git a/openspec/specs/roadmap-stage/spec.md b/openspec/specs/roadmap-stage/spec.md index 756404fde..39155bc50 100644 --- a/openspec/specs/roadmap-stage/spec.md +++ b/openspec/specs/roadmap-stage/spec.md @@ -2924,3 +2924,51 @@ The job SHALL judge the proposal that a split gives with the same call as the fi #### Scenario: A split names a registry item and not its test module - **WHEN** The split gives a group whose task names a registry item that a test module declares, and no task names the declared test module - **THEN** the job sends the reply back with the problem of the `registry` gate + +### Requirement: A staged item records its intent before the push +The job SHALL write the mark `stage.pending`, the branch name and the time as JSON, with one write of marks before it pushes the branch of an item. The write of the marks after the draft SHALL clear `stage.pending` with the other keys. + +#### Scenario: The request cost of a staged item +- **WHEN** a round stages one item +- **THEN** the round writes the marks twice for that item, and the record holds no `stage.pending` at the end + +### Requirement: An old intent mark with no recorded pull request gives an orphan +In each run the job SHALL look at the open items that hold `stage.pending` and no `stage.pr`, and SHALL use the branch name of the mark. A mark younger than 1 hour SHALL give no action. A branch that is not on `origin` SHALL give only the clear of the record. + +#### Scenario: The marks write fails +- **WHEN** the write of the marks after the draft fails, and a later round runs after the mark is 1 hour old +- **THEN** the round closes the draft, deletes the branch and clears the record, and a following round plans the item again + +#### Scenario: The worker dies after the push +- **WHEN** the worker dies after the push and before the draft, and a later round runs after the mark is 1 hour old +- **THEN** the round deletes the branch, makes no request to close a draft and clears the record + +#### Scenario: A retitle changes the slug +- **WHEN** a person changes the title of the item between the failure and the next round +- **THEN** the round still finds the orphan by the branch name of the mark + +#### Scenario: A young mark +- **WHEN** the mark is younger than 1 hour +- **THEN** the job makes no request and keeps the mark, and a round that runs when the mark is 1 hour old judges it + +#### Scenario: The branch is gone +- **WHEN** the branch of the mark is not on `origin` +- **THEN** the job clears the record and makes no request + +### Requirement: The job removes an orphan only when it wrote the branch alone +When every commit of the pushed branch of an orphan past the default branch is by the identity of the clone or is a clean merge, the job SHALL close the open pull request whose head is the branch, delete the branch and clear the record. The job SHALL read the open pull requests once in a round, and only when such a branch exists. When any other commit exists, the job SHALL keep the branch and the draft and say so in the plan. + +#### Scenario: A person added a commit +- **WHEN** the pushed branch of the mark holds a commit by another author +- **THEN** the job keeps the branch and the draft, makes no request and the plan says why + +#### Scenario: The request cost of a removal +- **WHEN** a round removes two orphans +- **THEN** the round reads the open pull requests once + +### Requirement: A branch with no intent mark is never touched +The job SHALL NOT close, delete or change a pushed branch or a pull request whose item holds no `stage.pending`. + +#### Scenario: A branch with no mark +- **WHEN** a person or an agent pushed a branch for another item that holds no `stage.pending` +- **THEN** the job does not touch the branch or its pull request