From ee27b98c62d969a4eaec495ee66f1fcfbe9f6815 Mon Sep 17 00:00:00 2001 From: Daniel Petty Date: Sat, 7 Nov 2026 20:38:34 +0000 Subject: [PATCH 1/5] docs(openspec): plan #4034 write each stage and alternatives item at once --- .../.openspec.yaml | 2 + .../design.md | 21 ++++++++ .../proposal.md | 52 +++++++++++++++++++ .../specs/roadmap-alternatives/spec.md | 22 ++++++++ .../specs/roadmap-stage/spec.md | 37 +++++++++++++ .../tasks.md | 50 ++++++++++++++++++ 6 files changed, 184 insertions(+) create mode 100644 openspec/changes/4034-write-each-stage-alternatives-item/.openspec.yaml create mode 100644 openspec/changes/4034-write-each-stage-alternatives-item/design.md create mode 100644 openspec/changes/4034-write-each-stage-alternatives-item/proposal.md create mode 100644 openspec/changes/4034-write-each-stage-alternatives-item/specs/roadmap-alternatives/spec.md create mode 100644 openspec/changes/4034-write-each-stage-alternatives-item/specs/roadmap-stage/spec.md create mode 100644 openspec/changes/4034-write-each-stage-alternatives-item/tasks.md diff --git a/openspec/changes/4034-write-each-stage-alternatives-item/.openspec.yaml b/openspec/changes/4034-write-each-stage-alternatives-item/.openspec.yaml new file mode 100644 index 000000000..e582ee4a2 --- /dev/null +++ b/openspec/changes/4034-write-each-stage-alternatives-item/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-11-07 diff --git a/openspec/changes/4034-write-each-stage-alternatives-item/design.md b/openspec/changes/4034-write-each-stage-alternatives-item/design.md new file mode 100644 index 000000000..37b9a8391 --- /dev/null +++ b/openspec/changes/4034-write-each-stage-alternatives-item/design.md @@ -0,0 +1,21 @@ +# Design + +## Decisions + +- **Two adapter classes, one in `stage.py` and one in `alternatives.py`.**: Each row writes a different set of artifacts, so a shared type would hold the union of both. `RefineWriter` is tied to `RefinedItem`, `RefineFailure` and `_apply_result`. +- **`plan_stage` and `plan_alternatives` take an optional writer.**: The loop holds the result of each item the moment it is done. A callback adds no thread and no second pass. With no writer the plan stays read-only, so `--plan` makes no write. +- **Stage order: unselected writes, then plan and write each selected item.**: The removals and the rejections of unselected items need no model, so they go before the first plan call. A stop then loses only the unfinished items. +- **A rejection of a selected item goes after its plan and before its publish.**: The flag `staged` of `_reject_plan` is then true only when the plan is ready, and the branch of the rejected plan is deleted before the new push. +- **The moved paths are read for one item, just before its publish.**: The read is git only. It sends no GitHub request, and it sees a commit that landed during the plans of the earlier items. +- **A stop of the Service and rejected credentials end a stage round.**: Both make every later request fail. The round records the failures of the finished items, then raises. Every other error is the failure of one item. +- **Alternatives: write check N before check N+1, and end on a rate limit.**: A kill loses only the check in progress. A rate limit blocks every later write, so the round must not spend model tokens on the other items. + +## Rejected alternatives + +- **One shared Protocol for both rows.**: `RefineWriter` is tied to its own types, so the shared type would carry the union of what stage and alternatives write. +- **Extend `RefineWriter` to all three rows.**: Stage writes publish, return, rejection and moved paths, and alternatives writes a body block, a record and a comment. `RefineWriter` holds none of these. +- **Keep the five-phase `apply_stage` after all plans.**: A stop loses every finished plan of the round. This is the defect. +- **Plan and write every item, including the unselected ones.**: Removals and rejections need no model, so a plan call for them wastes tokens and time. +- **End the round on every error.**: One error of one item would drop the finished items of the round. +- **End the round on no error.**: A stop or a refused identity would waste the rest of the request window. +- **Retry a rate-limited write until it succeeds.**: A rate limit blocks every later write, so the round cannot finish in the request window. diff --git a/openspec/changes/4034-write-each-stage-alternatives-item/proposal.md b/openspec/changes/4034-write-each-stage-alternatives-item/proposal.md new file mode 100644 index 000000000..0d3f2bb1d --- /dev/null +++ b/openspec/changes/4034-write-each-stage-alternatives-item/proposal.md @@ -0,0 +1,52 @@ +# Proposal + +## Why + +The stage row and the alternatives row of the Service plan every selected item first and write the results after. A stop of the Service, or a kill of the worker when the drain time ends, loses every finished plan of the round. A stage plan can use up to 1.5 million input tokens, so the next round pays for the same work again. #3631 removed this fault for the refine row only (#4034). + +Two more faults come from the same order: + +- `_plan_each` in `src/devops_cli/roadmap/stage/stage.py` does not catch `ServiceStoppingError`. The error leaves the loop and every finished plan is lost. +- `apply_alternatives` in `src/devops_cli/roadmap/alternatives.py` has no handler for `GitHubRateLimitError`. The round goes on to plan every item although every later write fails. + +## What Changes + +- The stage row plans one item, writes its result at once, and then plans the next item. The removals and the rejections of the items that the round does not select are written before the first plan call, because they need no model. A rejection of a selected item is written after its plan and before its publish. The moved paths are read for one item, just before its publish. +- A stop of the Service or rejected GitHub credentials end the round. The failures of the finished items are recorded first. Any other error is the failure of one item, and the round goes on. +- The alternatives row writes the body block, the record and the comment of check N before check N+1 starts. A write that fails with a rate limit ends the round and keeps the checks that are written. +- `plan_stage` and `plan_alternatives` take an optional writer, as `plan_refine` takes a `RefineWriter`. With no writer they write nothing, so `--plan` makes no GitHub write. `devops roadmap stage --confirm`, `devops roadmap alternatives --confirm` and the two rows use the same writers. +- The stage writer and the alternatives writer are two adapter classes, one in each module. The stage and alternatives specs gain requirements because the Service behavior changes. +- The change fits one pull request. + +## Capabilities + +### New Capabilities + +None. + +### Modified Capabilities + +- `roadmap-stage`: A round writes each item when the item is done +- `roadmap-alternatives`: A round writes each check when the check is done + +## Impact + +- Code: `src/devops_cli/roadmap/stage/stage.py`, `src/devops_cli/roadmap/alternatives.py`, `src/devops_cli/roadmap/run.py`, `src/devops_cli/commands/roadmap.py` +- Tests: `tests/roadmap/stage/test_roadmap_stage_item_writes.py`, `tests/roadmap/test_roadmap_alternatives_item_writes.py` + +## Feasibility + +- I read the code at origin/main. `plan_stage` calls `_plan_each` over all selected targets. `_plan_each` catches `AICredentialsError` (and re-raises it), `AIClientError`, `RoadmapStageError` and `ToolExecutionError` only, so `ServiceStoppingError` leaves the loop. `apply_stage` runs five phases after all plans and calls `_moved_by_item` once for the plan. `_run_stage_adapter` in `src/devops_cli/roadmap/run.py` is `plan_stage` and then `apply_stage`. +- `plan_alternatives` loops `_check_one` over all items, and `apply_alternatives` writes after the loop. It has no handler for `GitHubRateLimitError`. `_run_alternatives_adapter` is `plan_alternatives` and then `apply_alternatives`. +- The pattern exists and is merged: `RefineWriter` in `src/devops_cli/roadmap/refine/refine.py` with `begin`, `settle`, `end` and `raise_for_failures`, and `plan_refine(writer=None)` (#3631). `RefineWriter` is tied to `RefinedItem`, `RefineFailure` and `_apply_result`, so it does not fit stage or alternatives. Each row needs its own adapter. +- The new tests ran against origin/main. The kill test, the stop test and the order test of the stage row fail there, because item 1 has no draft pull request until `apply_stage` runs, and because the first plan call comes before the first removal. The kill test and the rate-limit test of the alternatives row fail there, because check 1 is not written when check 3 is killed, and because the round asks the model about all five items after the write failed. `devops ci fail-to-pass --base origin/main` runs them. +- Three tests are guards that pass on origin/main: the plan-only test of each row, and the test of the rejection of a selected item. They keep the old behavior (a plan makes no write, and the rejection comes between the plan and the publish) while the order changes. +- No merged change and no open pull request does this work, and no ADR or newer issue replaces it. + +## Existing Tool Considered + +Existing project code. No open library covers the write order of two roadmap rows. The nearest code is `RefineWriter` (#3631). The change follows its pattern with two adapter classes, and it deletes the five-phase `apply_stage` as a batch over a whole plan and the batch loop of `apply_alternatives`. The written cost is about 80k tokens and 3.5 hours to build with the tests, about 3k tokens a quarter to keep, and about 20k tokens and 1 hour for each fault. + +## Dependencies + +None. diff --git a/openspec/changes/4034-write-each-stage-alternatives-item/specs/roadmap-alternatives/spec.md b/openspec/changes/4034-write-each-stage-alternatives-item/specs/roadmap-alternatives/spec.md new file mode 100644 index 000000000..c82aa1d8d --- /dev/null +++ b/openspec/changes/4034-write-each-stage-alternatives-item/specs/roadmap-alternatives/spec.md @@ -0,0 +1,22 @@ +## ADDED Requirements + +### Requirement: An alternatives round writes each check when the check is done +An alternatives round SHALL write the body block, the record and the comment of check N before check N+1 starts. A kill of the worker SHALL lose at most the check in progress. + +#### Scenario: Kill in item 3 of 5 +- **WHEN** a round selects 5 items and the model call of the third item raises a `BaseException` that no handler catches +- **THEN** checks 1 and 2 have their body block, record and comment, and the other items have none + +### Requirement: A rate limit ends an alternatives round and keeps the written checks +An alternatives round SHALL end when a write fails with a GitHub rate limit. It SHALL keep the checks that it wrote and SHALL NOT check the items after the failed one. + +#### Scenario: Rate limit in check 2 +- **WHEN** the write of the body of item 2 of 5 fails with a rate-limit error +- **THEN** the round raises the error, item 1 keeps its written check, and the model gets no request for items 3, 4 and 5 + +### Requirement: An alternatives plan writes nothing +`devops roadmap alternatives --plan` SHALL make no write to the roadmap store. + +#### Scenario: Plan only +- **WHEN** the command runs with `--plan` on 3 items that have no check +- **THEN** the store receives no write diff --git a/openspec/changes/4034-write-each-stage-alternatives-item/specs/roadmap-stage/spec.md b/openspec/changes/4034-write-each-stage-alternatives-item/specs/roadmap-stage/spec.md new file mode 100644 index 000000000..5d2f65d2b --- /dev/null +++ b/openspec/changes/4034-write-each-stage-alternatives-item/specs/roadmap-stage/spec.md @@ -0,0 +1,37 @@ +## ADDED Requirements + +### Requirement: A stage round writes each item when the item is done +A stage round SHALL plan one selected item, write its result, and only then plan the next item. The result is the publish of a ready change, the return of a blocked item, or the stop comment of an item over the budget. A kill of the worker SHALL lose at most the item in progress. + +#### Scenario: Kill between item 1 and item 2 +- **WHEN** a round selects 3 items and the model call of item 2 raises a `BaseException` that no handler catches +- **THEN** item 1 has its pushed branch, its draft pull request, its record and its comment, and items 2 and 3 have no branch, no record and no comment + +### Requirement: A stop ends a stage round after it records the finished items +A stop of the Service or rejected GitHub credentials SHALL end a stage round. The round SHALL record the failure of each item that failed before the stop, and SHALL keep the written result of each finished item. Any other error SHALL be the failure of one item, and the round SHALL go on. + +#### Scenario: Stop of the Service in item 3 +- **WHEN** item 1 is staged, the plan of item 2 fails, and the Service stops in the plan of item 3 of 4 items +- **THEN** item 1 stays staged, item 2 has a failure record, items 3 and 4 have no record and no comment, and the round ends with the stop + +### Requirement: A stage round writes the unselected items first +A stage round SHALL write the removals of closed items and the rejections of items that it does not select before its first plan call. A rejection of a selected item SHALL be written after the plan of that item and before its publish. The round SHALL read the moved paths of an item just before it publishes that item. + +#### Scenario: Removal and rejection of unselected items +- **WHEN** a round has a closed item with a staged branch, an item with a rejected plan that the round leaves out, and one item to plan +- **THEN** the removal and the rejection are written before the first plan call + +#### Scenario: Rejection of a selected item +- **WHEN** a person names an item whose plan pull request was closed with no merge +- **THEN** the rejection is written after the plan of the item and before its publish, the staged branch of the rejected plan is deleted before the new push, and the Status of the item stays Ready + +#### Scenario: A commit lands during the plan of item 2 +- **WHEN** a commit that changes a path named by both plans lands on the default branch during the plan of item 2 +- **THEN** the comment of item 1 lists no moved path and the comment of item 2 lists the path + +### Requirement: A stage plan writes nothing +`devops roadmap stage --plan` SHALL make no write to GitHub or to the remote repository, also when the round holds a removal or a rejection. + +#### Scenario: Plan only +- **WHEN** the command runs with `--plan` on a roadmap that holds a removal, a rejection and a ready item +- **THEN** the store receives no write, no branch is pushed or deleted, and no GitHub write request is sent diff --git a/openspec/changes/4034-write-each-stage-alternatives-item/tasks.md b/openspec/changes/4034-write-each-stage-alternatives-item/tasks.md new file mode 100644 index 000000000..a4648de2f --- /dev/null +++ b/openspec/changes/4034-write-each-stage-alternatives-item/tasks.md @@ -0,0 +1,50 @@ +# Tasks + +## 1. The stage row writes each item when that item is done + +Kind: behavior. + +Reads: `src/devops_cli/roadmap/stage/stage.py`, `src/devops_cli/roadmap/run.py`, `src/devops_cli/commands/roadmap.py`, `src/devops_cli/roadmap/refine/refine.py`. Changes: `src/devops_cli/roadmap/stage/stage.py`, `src/devops_cli/roadmap/run.py`, `src/devops_cli/commands/roadmap.py`. Creates: `tests/roadmap/stage/test_roadmap_stage_item_writes.py`. + +- [ ] 1.1 In `tests/roadmap/stage/test_roadmap_stage_item_writes.py`, write the cases: a kill between item 1 and item 2, a stop of the Service in item 3 with the failure of item 2 recorded, the order of the removals, the rejections, the plan, the publish and the moved paths, and a plan that writes nothing + - Verification: `$ uv run devops ci test tests/roadmap/stage/test_roadmap_stage_item_writes.py` fails on the base +- [ ] 1.2 In `src/devops_cli/roadmap/stage/stage.py`, add the stage writer with the phase order of the design and the optional `writer` of `plan_stage`. Delete the five-phase batch of `apply_stage` and the single call of `_moved_by_item` for a whole plan. In `src/devops_cli/roadmap/run.py` and `src/devops_cli/commands/roadmap.py`, run `--confirm` and the row with the writer, and `--plan` with none + - Verification: `$ uv run devops ci test tests/roadmap/stage/test_roadmap_stage_item_writes.py` passes +- [ ] 1.3 Keep the other stage tests green with the new order + - Verification: `$ uv run devops ci test tests/roadmap/stage/test_roadmap_stage_apply.py` + +## 2. The alternatives row writes each check when that check is done + +Kind: behavior. + +Reads: `src/devops_cli/roadmap/alternatives.py`, `src/devops_cli/roadmap/run.py`, `src/devops_cli/commands/roadmap.py`. Changes: `src/devops_cli/roadmap/alternatives.py`, `src/devops_cli/roadmap/run.py`, `src/devops_cli/commands/roadmap.py`. Creates: `tests/roadmap/test_roadmap_alternatives_item_writes.py`. + +- [ ] 2.1 In `tests/roadmap/test_roadmap_alternatives_item_writes.py`, write the cases: a kill in item 3 of 5, a rate limit in the write of check 2, and a plan that writes nothing + - Verification: `$ uv run devops ci test tests/roadmap/test_roadmap_alternatives_item_writes.py` fails on the base +- [ ] 2.2 In `src/devops_cli/roadmap/alternatives.py`, add the alternatives writer and the optional `writer` of `plan_alternatives`, end the round on a rate limit, and delete the batch loop of `apply_alternatives`. In `src/devops_cli/roadmap/run.py` and `src/devops_cli/commands/roadmap.py`, run `--confirm` and the row with the writer, and `--plan` with none + - Verification: `$ uv run devops ci test tests/roadmap/test_roadmap_alternatives_item_writes.py` passes +- [ ] 2.3 Keep the other alternatives tests green with the new order + - Verification: `$ uv run devops ci test tests/roadmap/test_roadmap_alternatives.py` + +## 3. The documents match the new order + +Kind: documents. + +Changes: the documents that state "plan all, then write" for the stage and alternatives rows, if any. + +- [ ] 3.1 Fix each document that says the stage row or the alternatives row writes after it plans all items + - Verification: `$ uv run devops ci docs` + +## Criteria + +- Group 1: A stage round SHALL write the result of each item before it plans the next item. +- Group 1: A stop of the Service SHALL end a stage round after it records the failures of the finished items. +- Group 1: A stage round SHALL write the removals and the rejections of unselected items before its first plan call. +- Group 1: A stage plan SHALL write nothing. +- Group 2: An alternatives round SHALL write the check of an item before it checks the next item. +- Group 2: A rate limit SHALL end an alternatives round and keep the written checks. +- Group 2: An alternatives plan SHALL write nothing. + +## Workflow follow-up + +- Pending a person: review the diff in tuicr (tuicr -r ..HEAD). From d642f5f514bab1cad497983869292e7d30ade931 Mon Sep 17 00:00:00 2001 From: Daniel Petty Date: Sat, 7 Nov 2026 20:40:46 +0000 Subject: [PATCH 2/5] test(roadmap): stage and alternatives rows write each item when it is done --- .../stage/test_roadmap_stage_item_writes.py | 452 ++++++++++++++++++ .../test_roadmap_alternatives_item_writes.py | 188 ++++++++ 2 files changed, 640 insertions(+) create mode 100644 tests/roadmap/stage/test_roadmap_stage_item_writes.py create mode 100644 tests/roadmap/test_roadmap_alternatives_item_writes.py diff --git a/tests/roadmap/stage/test_roadmap_stage_item_writes.py b/tests/roadmap/stage/test_roadmap_stage_item_writes.py new file mode 100644 index 000000000..3eb29f68d --- /dev/null +++ b/tests/roadmap/stage/test_roadmap_stage_item_writes.py @@ -0,0 +1,452 @@ +"""A stage round writes each item when that item is done (#4034). + +A kill of the worker or a stop of the Service ends the round and loses at most the item in +progress. The removals and the rejections of the items that the round does not select are written +before the first plan call, a rejection of a selected item between its plan and its publish, and +the moved paths of an item are read just before its publish. A plan with `--plan` writes nothing. + +The roadmap is the in-memory store, the model edge is the fake, GitHub is answered at its HTTP edge +and git runs on a temporary repository. The rounds run through the row of the Service +(`_run_stage_adapter`) and the commands, so no test names a writer. +""" + +from __future__ import annotations + +import json +from collections.abc import Callable +from datetime import UTC, datetime +from typing import TYPE_CHECKING, Any + +import pytest + +from devops_cli.config.constants import ( + CONST_ROADMAP_STAGE_COMMENT_MARKER, + CONST_ROADMAP_STAGE_REJECTED_MARKER, +) +from devops_cli.exceptions import RoadmapStageError, ServiceStoppingError, ended_by_stop +from devops_cli.lang import MESSAGES +from devops_cli.roadmap.body_hash import body_hashes +from devops_cli.roadmap.run import _run_stage_adapter +from devops_cli.roadmap.stores.store import CloseReason, ItemField, StageRecordKey +from tests.support.builders import make_stage_proposal +from tests.support.fakes.github import github_json +from tests.support.fakes.roadmap_store import InMemoryRoadmapStore +from tests.support.roadmap_stage import ( + STAGE_NAME, + STAGE_REPO, + VALID_REPORT, + StageRepo, + advance_origin, + no_findings, + seed_stageable, + validate_args, +) +from tests.support.service_stop import stop_error + +if TYPE_CHECKING: + from pathlib import Path + + 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") + +Cli = Callable[..., Any] + +PULLS = f"/repos/{STAGE_REPO}/pulls" +TOPICS = ("comment", "label", "milestone", "assignee", "reviewer") +CLOSED_AT = datetime(2026, 11, 2, 20, 22, 45, tzinfo=UTC) +EVIDENCE = ( + "The premise does not hold: `src/devops_cli/roadmap/runner_tools.py` names `bwrap` in " + "`NOT_IN_RUNNER`, so the runner can run no review. I ran the command and it failed closed." +) +MOVED = {"src/devops_cli/sample.py": "def keep(comments):\n return comments\n"} + + +class WorkerKilled(BaseException): + """The kill of the worker: a `BaseException`, so no handler of the job catches it.""" + + +def _title(number: int) -> str: + return f"fix(roadmap): the job drops a {TOPICS[number - 1]}" + + +def _branch(number: int) -> str: + return f"fix/{number}-the-job-drops-a-{TOPICS[number - 1]}" + + +def _opened(number: int) -> Any: + return github_json( + {"number": number, "html_url": f"https://example.com/pull/{number}"}, status=201 + ) + + +def _roadmap(count: int, openspec: OpenSpecFake, api: GitHubApiFake) -> InMemoryRoadmapStore: + """`count` Ready bugs, numbered from 1, and the draft pull request that each one gets. GitHub + numbers the pull requests after the issues, in the order the round opens them.""" + store = InMemoryRoadmapStore(repo=STAGE_REPO) + store.seed_file( + ".github/labels.yml", + '- name: "type/docs"\n color: "0075ca"\n- name: "scope/openspec"\n color: "0e8a16"\n', + ) + for number in range(1, count + 1): + seed_stageable(store, _title(number)) + openspec.script_sequence( + validate_args(_branch(number).removeprefix("fix/")), (0, VALID_REPORT) + ) + pulls = [ + store.seed_pull("staging PR", head_ref=_branch(n), draft=True) for n in range(1, count + 1) + ] + api.route("POST", PULLS, *(_opened(pull) for pull in pulls)) + return store + + +def _round(store: InMemoryRoadmapStore, repo: StageRepo) -> Any: + return _run_stage_adapter(store, repo=STAGE_REPO, clone_path=repo.clone) + + +def _number_of(prompt: str) -> int | None: + """The item a planner request is about, and None for the request of the verification pass.""" + request = json.loads(prompt) + return None if "plan" in request else int(request["number"]) + + +def _pushed(repo: StageRepo) -> list[str]: + return repo.origin_git("branch", "--format=%(refname:short)").split() + + +def _record(store: InMemoryRoadmapStore, number: int) -> dict[str, str]: + item = store.item(number) + assert item is not None + return {k.value: v for k, v in item.job_record.items() if v is not None} + + +def _staged_comments(store: InMemoryRoadmapStore, number: int) -> list[str]: + return [ + c.body + for c in store.comments_on(number) + if c.body.startswith(CONST_ROADMAP_STAGE_COMMENT_MARKER) + ] + + +class _Journal(InMemoryRoadmapStore): + """A store that keeps the order of the writes of a round with the plan calls that the model + receives: `events` holds ("plan", n) for the plan of item n, ("comment", n, kind) and + ("marks", n) for the writes the job makes as a job.""" + + def __init__(self, **options: Any) -> None: + super().__init__(**options) + self.events: list[tuple[Any, ...]] = [] + + def comment(self, number: int, body: str) -> None: + kind = "rejected" if CONST_ROADMAP_STAGE_REJECTED_MARKER in body else "staged" + self.events.append(("comment", number, kind)) + super().comment(number, body) + + def set_marks(self, item: Any, marks: Any) -> None: + self.events.append(("marks", item.number)) + super().set_marks(item, marks) + + def set_field(self, item: Any, field: ItemField, value: str | None, **options: Any) -> None: + self.events.append(("field", item.number)) + super().set_field(item, field, value, **options) + + +def _journaled(count: int, openspec: OpenSpecFake, api: GitHubApiFake) -> _Journal: + """The roadmap of `_roadmap` in a store that keeps a journal.""" + store = _Journal(repo=STAGE_REPO) + store.seed_file( + ".github/labels.yml", + '- name: "type/docs"\n color: "0075ca"\n- name: "scope/openspec"\n color: "0e8a16"\n', + ) + for number in range(1, count + 1): + seed_stageable(store, _title(number)) + for number in range(1, count + 1): + openspec.script_sequence( + validate_args(_branch(number).removeprefix("fix/")), (0, VALID_REPORT) + ) + return store + + +# ── A kill or a stop of the Service ─────────────────────────────────────────── + + +def test_killed_between_items_the_first_item_is_written_and_the_others_have_nothing( + stage_repo: StageRepo, + model_fake: ModelFake, + github_api: GitHubApiFake, + openspec_fake: OpenSpecFake, +) -> None: + store = _roadmap(3, openspec_fake, github_api) + no_findings(model_fake) + + def plan(prompt: str) -> Any: + if _number_of(prompt) == 2: + raise WorkerKilled("the worker was killed") + return make_stage_proposal() + + model_fake.answer(plan) + + with pytest.raises(WorkerKilled, match="worker was killed"): + _round(store, stage_repo) + + assert ( + _pushed(stage_repo), + [json.loads(r.content)["head"] for r in github_api.sent if r.method == "POST"], + _record(store, 1).get(StageRecordKey.PR.value), + len(_staged_comments(store, 1)), + [_record(store, n).get(StageRecordKey.BRANCH.value) for n in (2, 3)], + [len(store.comments_on(n)) for n in (2, 3)], + ) == ([_branch(1), "main"], [_branch(1)], "4", 1, [None, None], [0, 0]) + + +def test_service_stopping_in_item_three_keeps_item_one_and_records_the_failure_of_item_two( + stage_repo: StageRepo, + model_fake: ModelFake, + github_api: GitHubApiFake, + openspec_fake: OpenSpecFake, +) -> None: + """Item 1 is staged, the plan of item 2 breaks a rule of the stage job and so fails that item, + and the stop of the Service falls in the plan of item 3. Item 4 is never taken.""" + store = _roadmap(4, openspec_fake, github_api) + no_findings(model_fake) + + def plan(prompt: str) -> Any: + number = _number_of(prompt) + if number == 2: + return make_stage_proposal(pull_request_body="Fixes #4 now.") + if number == 3: + raise stop_error() + return make_stage_proposal() + + model_fake.answer(plan) + + with pytest.raises(ServiceStoppingError) as stopped: + _round(store, stage_repo) + + assert ( + ended_by_stop(stopped.value), + _pushed(stage_repo), + _record(store, 1).get(StageRecordKey.PR.value), + len(_staged_comments(store, 1)), + _record(store, 2).get(StageRecordKey.FAILED_ERROR.value), + [StageRecordKey.FAILED_ERROR.value in _record(store, n) for n in (1, 3, 4)], + [len(store.comments_on(n)) for n in (2, 3, 4)], + ) == ( + True, + [_branch(1), "main"], + "5", + 1, + RoadmapStageError.__name__, + [False, False, False], + [0, 0, 0], + ) + + +# ── The order of the writes ─────────────────────────────────────────────────── + + +@pytest.fixture +def service_app(config: Callable[..., object]) -> None: + """The Service App has the bot login `devops-roadmap[bot]`.""" + config(service={"github_app": {"slug": "devops-roadmap"}}) + + +def _rejected(store: InMemoryRoadmapStore, api: GitHubApiFake, number: int) -> int: + """Item `number` has a plan pull request that a person closed with no merge and a reason.""" + pull = store.seed_pull("docs(openspec): plan", head_ref=_branch(number)) + item = store.item(number) + assert item is not None + store.set_marks( + item, + { + StageRecordKey.BRANCH: _branch(number), + StageRecordKey.PR: str(pull), + StageRecordKey.READY: str(pull), + StageRecordKey.SHA: "abc", + StageRecordKey.GROUPS: json.dumps([["Group", 100]]), + StageRecordKey.TEXT_HASH: body_hashes(item.body)[0], + }, + ) + store.seed_comment(pull, EVIDENCE, author_association="OWNER", author_login="alice") + api.route( + "GET", + f"{PULLS}/{pull}", + github_json( + { + "state": "closed", + "merged": False, + "draft": False, + "closed_at": CLOSED_AT.isoformat(), + "head": {"sha": "abc", "ref": _branch(number)}, + } + ), + ) + return pull + + +@pytest.mark.usefixtures("service_app") +def test_phase_order_removals_and_rejections_of_unselected_items_come_before_the_first_plan_call( + stage_repo: StageRepo, + model_fake: ModelFake, + github_api: GitHubApiFake, + openspec_fake: OpenSpecFake, +) -> None: + """Item 1 is closed and keeps the record of a branch that is gone: a removal. Item 2 has a plan + that a person rejected: the round leaves it out. Item 3 is the one item the round plans.""" + store = _journaled(3, openspec_fake, github_api) + store.seed_pull("staging PR", head_ref=_branch(1)) # pull 4 + store.seed_pull("staging PR", head_ref=_branch(2)) # pull 5 + store.seed_pull("staging PR", head_ref=_branch(3), draft=True) # pull 6 + one = store.item(1) + assert one is not None + store.set_marks(one, {StageRecordKey.BRANCH: _branch(1), StageRecordKey.PR: "4"}) + store.close_by_hand(1, CloseReason.COMPLETED) + _rejected(store, github_api, 2) + github_api.route("POST", PULLS, _opened(6)) + store.events.clear() + no_findings(model_fake) + + def plan(prompt: str) -> Any: + number = _number_of(prompt) + store.events.append(("plan", number)) + return make_stage_proposal() + + model_fake.answer(plan) + + _round(store, stage_repo) + + assert store.events == [ + ("marks", 1), + ("comment", 2, "rejected"), + ("field", 2), + ("plan", 3), + ("marks", 3), + ("comment", 3, "staged"), + ] + + +@pytest.mark.usefixtures("service_app") +def test_phase_order_a_rejection_of_a_selected_item_falls_between_its_plan_and_its_publish( + cli: Cli, + stage_repo: StageRepo, + model_fake: ModelFake, + github_api: GitHubApiFake, + openspec_fake: OpenSpecFake, + roadmap_store: InMemoryRoadmapStore, +) -> None: + """A person names the item whose plan they rejected, so the round plans it again. The staged + branch of the rejected plan goes before the new push and the Status of the item stays Ready.""" + openspec_fake.script_sequence(validate_args(_branch(1).removeprefix("fix/")), (0, VALID_REPORT)) + roadmap_store.seed_file( + ".github/labels.yml", + '- name: "type/docs"\n color: "0075ca"\n- name: "scope/openspec"\n color: "0e8a16"\n', + ) + seed_stageable(roadmap_store, _title(1)) + pull = _rejected(roadmap_store, github_api, 1) + stage_repo.push_branch(_branch(1), author=STAGE_NAME) + github_api.route("POST", PULLS, _opened(pull + 1)) + roadmap_store.seed_pull("staging PR", head_ref=_branch(1), draft=True) + order: list[str] = [] + no_findings(model_fake) + + def plan(prompt: str) -> Any: + order.append(f"plan:{_branch(1) in _pushed(stage_repo)}") + return make_stage_proposal() + + model_fake.answer(plan) + + result = cli( + "roadmap", "stage", "--repo", STAGE_REPO, "--source", str(stage_repo.clone), + "--item", "1", "--confirm", + ) # fmt: skip + + item = roadmap_store.item(1) + assert item is not None + assert result.exit_code == 0, result.output + assert ( + order, + item.status, + _record(roadmap_store, 1).get(StageRecordKey.PR.value), + StageRecordKey.REJECTED.value in _record(roadmap_store, 1), + [ + CONST_ROADMAP_STAGE_REJECTED_MARKER in c.body + for c in roadmap_store.comments_on(1) + if c.body.startswith(CONST_ROADMAP_STAGE_COMMENT_MARKER) + ], + ) == ( + ["plan:True"], + "Ready", + str(pull + 1), + False, + [True, False], + ) + assert _pushed(stage_repo) == [_branch(1), "main"] + + +def test_phase_order_the_moved_paths_are_read_for_each_item_just_before_its_publish( + stage_repo: StageRepo, + model_fake: ModelFake, + github_api: GitHubApiFake, + openspec_fake: OpenSpecFake, + tmp_path: Path, +) -> None: + """A commit that changes a named path lands on the default branch during the plan of item 2. + Item 1 was written before it landed, so its comment lists no moved path. Item 2 is written + after, so its comment does.""" + store = _roadmap(2, openspec_fake, github_api) + no_findings(model_fake) + + def plan(prompt: str) -> Any: + if _number_of(prompt) == 2: + advance_origin(stage_repo, tmp_path, MOVED) + return make_stage_proposal() + + model_fake.answer(plan) + + _round(store, stage_repo) + + assert [ + MESSAGES.roadmap.stage_comment_moved in _staged_comments(store, n)[0] for n in (1, 2) + ] == [False, True] + + +# ── A plan writes nothing ───────────────────────────────────────────────────── + + +@pytest.mark.usefixtures("service_app") +def test_plan_only_the_command_makes_no_write_for_a_removal_a_rejection_or_a_ready_item( + cli: Cli, + stage_repo: StageRepo, + model_fake: ModelFake, + github_api: GitHubApiFake, + openspec_fake: OpenSpecFake, + roadmap_store: InMemoryRoadmapStore, +) -> None: + for number in range(1, 4): + seed_stageable(roadmap_store, _title(number)) + roadmap_store.seed_pull("staging PR", head_ref=_branch(1)) + roadmap_store.seed_pull("staging PR", head_ref=_branch(2)) + one = roadmap_store.item(1) + assert one is not None + roadmap_store.set_marks(one, {StageRecordKey.BRANCH: _branch(1), StageRecordKey.PR: "4"}) + roadmap_store.close_by_hand(1, CloseReason.COMPLETED) + _rejected(roadmap_store, github_api, 2) + openspec_fake.script_sequence(validate_args(_branch(3).removeprefix("fix/")), (0, VALID_REPORT)) + before = len(roadmap_store.writes) + no_findings(model_fake) + model_fake.answer(make_stage_proposal()) + + result = cli( + "roadmap", "stage", "--repo", STAGE_REPO, "--source", str(stage_repo.clone), "--plan" + ) + + assert result.exit_code == 0, result.output + assert ( + roadmap_store.writes[before:], + [r.method for r in github_api.sent if r.method != "GET"], + _pushed(stage_repo), + [item.status for item in map(roadmap_store.item, (2, 3)) if item], + ) == ([], [], ["main"], ["Ready", "Ready"]) + assert "# Stage plan for example/roadmap" in result.output diff --git a/tests/roadmap/test_roadmap_alternatives_item_writes.py b/tests/roadmap/test_roadmap_alternatives_item_writes.py new file mode 100644 index 000000000..1420692bd --- /dev/null +++ b/tests/roadmap/test_roadmap_alternatives_item_writes.py @@ -0,0 +1,188 @@ +"""An alternatives round writes each check when that check is done (#4034). + +A kill of the worker or a rate limit on a write ends the round and loses at most the check in +progress: the body block, the record and the comment of check N are written before check N+1 +starts. A plan with `--plan` writes nothing. + +The roadmap is the in-memory store, pydantic-ai's `FunctionModel` answers each model request from a +script, and Firecrawl is answered at the HTTP edge. The rounds run through the row of the Service +(`_run_alternatives_adapter`) and the command, so no test names a writer. +""" + +from __future__ import annotations + +import json +from collections.abc import Callable +from typing import TYPE_CHECKING, Any + +import pytest +from pydantic_ai.messages import ModelMessage, ToolReturnPart, UserPromptPart +from typer.testing import CliRunner + +from devops_cli.commands.roadmap import app as roadmap_app +from devops_cli.config.constants import ( + CONST_ROADMAP_ALTERNATIVES_COMMENT_MARKER, + CONST_ROADMAP_ALTERNATIVES_START_MARKER, +) +from devops_cli.config.defaults import DEFAULT_FIRECRAWL_SEARCH_URL +from devops_cli.exceptions.git import GitHubRateLimitError +from devops_cli.roadmap.alternatives import AlternativesReply, Verdict +from devops_cli.roadmap.run import _run_alternatives_adapter +from devops_cli.roadmap.stores.store import AlternativesRecordKey, ItemField +from tests.support.fakes.roadmap_store import InMemoryRoadmapStore +from tests.support.scripted_model import Script, call + +if TYPE_CHECKING: + from tests.support.fakes.http import WebFake + +pytestmark = pytest.mark.usefixtures("keyring_store") + +REPO = "example/roadmap" +REASON = "No open tool covers the need." +SEARCH = call("search_web", query="scanner") + + +class WorkerKilled(BaseException): + """The kill of the worker: a `BaseException`, so no handler of the job catches it.""" + + +@pytest.fixture +def store() -> InMemoryRoadmapStore: + """A public roadmap with five open items that have no check.""" + roadmap = InMemoryRoadmapStore(repo=REPO, is_private=False) + for n in range(5): + roadmap.seed_issue( + f"Scan a directory {n}", + body=f"The job needs to read every file of directory {n}.", + fields={ItemField.PRIORITY: "P2-Medium", ItemField.STATUS: "New"}, + ) + return roadmap + + +@pytest.fixture(autouse=True) +def web_key(config: Callable[..., Any], monkeypatch: pytest.MonkeyPatch, web: WebFake) -> None: + """A Firecrawl key in the settings, and a Firecrawl that answers each search with one result.""" + for name in ("DEVOPS_CLI_FIRECRAWL_API_KEY", "FIRECRAWL_API_KEY"): + monkeypatch.delenv(name, raising=False) + config(search={"providers": ["firecrawl"]}, firecrawl={"api_key": "fc-test"}) + results = [ + {"title": "Result", "url": "https://example.com/scanner", "description": "A scanner."} + ] + web.content( + DEFAULT_FIRECRAWL_SEARCH_URL, + json.dumps({"success": True, "data": {"web": results}}).encode(), + ) + + +def _prompt_of(messages: list[ModelMessage]) -> dict[str, Any]: + text = next( + part.content + for message in messages + for part in getattr(message, "parts", ()) + if isinstance(part, UserPromptPart) + ) + document: dict[str, Any] = json.loads(str(text)) + return document + + +def _searched(messages: list[ModelMessage]) -> bool: + return any( + isinstance(part, ToolReturnPart) and part.tool_name == "search_web" + for message in messages + for part in getattr(message, "parts", ()) + ) + + +class _Model: + """A model that searches once for each item and then says that no tool covers it. It keeps the + numbers of the items it was asked about, in order, and raises `WorkerKilled` for the item + that is the `kill_at`-th it sees.""" + + def __init__(self, monkeypatch: pytest.MonkeyPatch, *, kill_at: int = 0) -> None: + self.asked: list[int] = [] + self.kill_at = kill_at + self.script = Script(self._answer) + monkeypatch.setattr( + "devops_cli.roadmap.alternatives.get_task_model", lambda _task: self.script.model() + ) + + def _answer(self, messages: list[ModelMessage]) -> Any: + number = _prompt_of(messages)["number"] + if number not in self.asked: + self.asked.append(number) + if len(self.asked) == self.kill_at: + raise WorkerKilled("the worker was killed") + if _searched(messages): + return AlternativesReply(reason=REASON, verdict=Verdict.NONE) + return SEARCH + + +def _written(store: InMemoryRoadmapStore, number: int) -> tuple[bool, bool, bool]: + """Whether item `number` has the body block, the record and the comment of a check.""" + item = store.item(number) + assert item is not None + return ( + CONST_ROADMAP_ALTERNATIVES_START_MARKER in store.read_issue_body(number), + AlternativesRecordKey.CHECKED_AT in item.job_record, + any( + c.body.startswith(CONST_ROADMAP_ALTERNATIVES_COMMENT_MARKER) + for c in store.comments_on(number) + ), + ) + + +def test_killed_between_items_the_first_two_checks_are_written_and_the_others_have_nothing( + store: InMemoryRoadmapStore, monkeypatch: pytest.MonkeyPatch +) -> None: + model = _Model(monkeypatch, kill_at=3) + + with pytest.raises(WorkerKilled, match="worker was killed"): + _run_alternatives_adapter(store, repo=REPO) + + first, second, _killed = model.asked + nothing = (False, False, False) + assert ( + len(model.asked), + _written(store, first), + _written(store, second), + [_written(store, n) for n in range(1, 6) if n not in (first, second)], + ) == (3, (True, True, True), (True, True, True), [nothing] * 3) + + +def test_rate_limit_in_the_write_of_check_two_ends_the_round_and_keeps_check_one( + store: InMemoryRoadmapStore, monkeypatch: pytest.MonkeyPatch +) -> None: + model = _Model(monkeypatch) + store.fail_write( + write="write_issue_body", + item=2, + error=GitHubRateLimitError("No requests are left.", operation="roadmap.write"), + ) + + with pytest.raises(GitHubRateLimitError, match="No requests are left"): + _run_alternatives_adapter(store, repo=REPO) + + nothing = (False, False, False) + assert ( + model.asked, + _written(store, 1), + [_written(store, n) for n in range(2, 6)], + ) == ([1, 2], (True, True, True), [nothing] * 4) + + +def test_plan_only_the_command_makes_no_write( + roadmap_store: InMemoryRoadmapStore, monkeypatch: pytest.MonkeyPatch +) -> None: + for n in range(3): + roadmap_store.seed_issue( + f"Scan a directory {n}", + body=f"The job needs to read every file of directory {n}.", + fields={ItemField.PRIORITY: "P2-Medium", ItemField.STATUS: "New"}, + ) + model = _Model(monkeypatch) + before = len(roadmap_store.writes) + + result = CliRunner().invoke(roadmap_app, ["alternatives", "--repo", REPO, "--plan"]) + + assert result.exit_code == 0, result.output + assert (roadmap_store.writes[before:], len(model.asked)) == ([], 3) From 0c191bee7f9bd6890401a2f37c6181fa0b5b9da2 Mon Sep 17 00:00:00 2001 From: Daniel Petty Date: Sat, 7 Nov 2026 21:00:31 +0000 Subject: [PATCH 3/5] fix(roadmap): the stage and alternatives rows write each item when it is done --- docs/roadmap/alternatives.md | 4 +- docs/roadmap/stage.md | 4 +- .../tasks.md | 14 +- src/devops_cli/commands/roadmap.py | 20 +- src/devops_cli/roadmap/alternatives.py | 83 +++--- src/devops_cli/roadmap/run.py | 23 +- src/devops_cli/roadmap/stage/stage.py | 257 +++++++++++------- .../roadmap/stage/test_roadmap_stage_apply.py | 2 +- .../stage/test_roadmap_stage_reconcile.py | 4 +- .../stage/test_roadmap_stage_rejected.py | 2 +- .../roadmap/stage/test_roadmap_stage_retry.py | 3 +- .../stage/test_roadmap_stage_verification.py | 2 +- tests/roadmap/test_roadmap_alternatives.py | 2 +- tests/support/roadmap_alternatives.py | 23 ++ tests/support/roadmap_stage.py | 29 ++ 15 files changed, 301 insertions(+), 171 deletions(-) create mode 100644 tests/support/roadmap_alternatives.py diff --git a/docs/roadmap/alternatives.md b/docs/roadmap/alternatives.md index b94bb9895..9d8b2d356 100644 --- a/docs/roadmap/alternatives.md +++ b/docs/roadmap/alternatives.md @@ -74,6 +74,8 @@ The check is off for a private repository and when no search provider is configu An item is skipped, reported and recorded in its job record when its model call fails, when its run stops, or when the job refuses its reply. The job writes the other items. The run takes a skipped item again after `roadmap.alternatives.retry_days`. +The job writes the check of an item before it checks the next item. A kill of the worker loses at most the check in progress. A rate limit on a write ends the round, and the checks that are written stay. + The command exits 1 only when it tried items and checked none. ## Modes @@ -81,7 +83,7 @@ The command exits 1 only when it tried items and checked none. | Flag | What the run does | | :--- | :--- | | `--plan` (the default) | Runs the checks and prints them. It writes nothing. | -| `--confirm` | Writes each check to GitHub: its comment, its body block and its job record. | +| `--confirm` | Writes each check to GitHub as soon as the item is checked: its comment, its body block and its job record. | | `--dry-run` | Makes no request. It prints the requests that a run makes, with placeholders. | ## Where the rules are diff --git a/docs/roadmap/stage.md b/docs/roadmap/stage.md index 2e6b9d89a..e48ac029c 100644 --- a/docs/roadmap/stage.md +++ b/docs/roadmap/stage.md @@ -12,6 +12,8 @@ For each item that a label filter names, the job does these things in order: 4. It opens a draft pull request. The title is `docs(openspec): plan # `. The body starts with `Plans #` and holds no closing keyword. `build_plan_body` of `pull_form` builds it, and a body that breaks the plan form goes back to the model once, as a refused plan does. It adds the `type/*` and `scope/*` labels of the title to the draft. 5. It writes one comment on the issue. The comment names the branch, the pull request, and the task groups with their measures. A label that GitHub refuses becomes a note in the comment. +The job writes each item when the item is planned, and it plans the next item after that. A stop of the Service loses at most the item in progress. Before the first plan, it removes the staged branches of closed items and writes the rejected plans of the items that the round does not select, because these need no model. It writes the rejection of a selected item after the plan of that item and before the push. It reads the paths that moved on the default branch for one item, just before its push. + The Status of the item stays Ready. The job does not mark the pull request ready. The Service does it, in its row `stage_ready`, when all checks of the draft have passed. The overlap read is one GraphQL request for each 100 open pull requests. The context lists each pull request whose files meet a path that the item body cites, with its number, title, state, plan mark and shared paths, and each other file that holds a cited metric name. The prompt writes each entry into `## Dependencies`, and the gate refuses a plan with an empty `dependencies` field when the context lists an overlap. The comment of the staged item states the number of GraphQL requests. @@ -93,7 +95,7 @@ Refine reads the comment as `stage_return`, because it is a comment of the stage | Flag | What the run does | | :--- | :--- | | `--plan` (the default) | Reads GitHub and calls the model. It prints the changes that `--confirm` writes. It writes nothing. | -| `--confirm` | Plans as `--plan` does, then makes the writes in git and on GitHub. | +| `--confirm` | Plans as `--plan` does, and makes the writes in git and on GitHub for each item as soon as the item is planned. | | `--dry-run` | Makes no request. It prints the requests and the git commands that a run makes, with placeholders. | ## Where the rules are diff --git a/openspec/changes/4034-write-each-stage-alternatives-item/tasks.md b/openspec/changes/4034-write-each-stage-alternatives-item/tasks.md index a4648de2f..72f1634f1 100644 --- a/openspec/changes/4034-write-each-stage-alternatives-item/tasks.md +++ b/openspec/changes/4034-write-each-stage-alternatives-item/tasks.md @@ -6,11 +6,11 @@ Kind: behavior. Reads: `src/devops_cli/roadmap/stage/stage.py`, `src/devops_cli/roadmap/run.py`, `src/devops_cli/commands/roadmap.py`, `src/devops_cli/roadmap/refine/refine.py`. Changes: `src/devops_cli/roadmap/stage/stage.py`, `src/devops_cli/roadmap/run.py`, `src/devops_cli/commands/roadmap.py`. Creates: `tests/roadmap/stage/test_roadmap_stage_item_writes.py`. -- [ ] 1.1 In `tests/roadmap/stage/test_roadmap_stage_item_writes.py`, write the cases: a kill between item 1 and item 2, a stop of the Service in item 3 with the failure of item 2 recorded, the order of the removals, the rejections, the plan, the publish and the moved paths, and a plan that writes nothing +- [x] 1.1 In `tests/roadmap/stage/test_roadmap_stage_item_writes.py`, write the cases: a kill between item 1 and item 2, a stop of the Service in item 3 with the failure of item 2 recorded, the order of the removals, the rejections, the plan, the publish and the moved paths, and a plan that writes nothing - Verification: `$ uv run devops ci test tests/roadmap/stage/test_roadmap_stage_item_writes.py` fails on the base -- [ ] 1.2 In `src/devops_cli/roadmap/stage/stage.py`, add the stage writer with the phase order of the design and the optional `writer` of `plan_stage`. Delete the five-phase batch of `apply_stage` and the single call of `_moved_by_item` for a whole plan. In `src/devops_cli/roadmap/run.py` and `src/devops_cli/commands/roadmap.py`, run `--confirm` and the row with the writer, and `--plan` with none +- [x] 1.2 In `src/devops_cli/roadmap/stage/stage.py`, add the stage writer with the phase order of the design and the optional `writer` of `plan_stage`. Delete the five-phase batch of `apply_stage` and the single call of `_moved_by_item` for a whole plan. In `src/devops_cli/roadmap/run.py` and `src/devops_cli/commands/roadmap.py`, run `--confirm` and the row with the writer, and `--plan` with none - Verification: `$ uv run devops ci test tests/roadmap/stage/test_roadmap_stage_item_writes.py` passes -- [ ] 1.3 Keep the other stage tests green with the new order +- [x] 1.3 Keep the other stage tests green with the new order - Verification: `$ uv run devops ci test tests/roadmap/stage/test_roadmap_stage_apply.py` ## 2. The alternatives row writes each check when that check is done @@ -19,11 +19,11 @@ Kind: behavior. Reads: `src/devops_cli/roadmap/alternatives.py`, `src/devops_cli/roadmap/run.py`, `src/devops_cli/commands/roadmap.py`. Changes: `src/devops_cli/roadmap/alternatives.py`, `src/devops_cli/roadmap/run.py`, `src/devops_cli/commands/roadmap.py`. Creates: `tests/roadmap/test_roadmap_alternatives_item_writes.py`. -- [ ] 2.1 In `tests/roadmap/test_roadmap_alternatives_item_writes.py`, write the cases: a kill in item 3 of 5, a rate limit in the write of check 2, and a plan that writes nothing +- [x] 2.1 In `tests/roadmap/test_roadmap_alternatives_item_writes.py`, write the cases: a kill in item 3 of 5, a rate limit in the write of check 2, and a plan that writes nothing - Verification: `$ uv run devops ci test tests/roadmap/test_roadmap_alternatives_item_writes.py` fails on the base -- [ ] 2.2 In `src/devops_cli/roadmap/alternatives.py`, add the alternatives writer and the optional `writer` of `plan_alternatives`, end the round on a rate limit, and delete the batch loop of `apply_alternatives`. In `src/devops_cli/roadmap/run.py` and `src/devops_cli/commands/roadmap.py`, run `--confirm` and the row with the writer, and `--plan` with none +- [x] 2.2 In `src/devops_cli/roadmap/alternatives.py`, add the alternatives writer and the optional `writer` of `plan_alternatives`, end the round on a rate limit, and delete the batch loop of `apply_alternatives`. In `src/devops_cli/roadmap/run.py` and `src/devops_cli/commands/roadmap.py`, run `--confirm` and the row with the writer, and `--plan` with none - Verification: `$ uv run devops ci test tests/roadmap/test_roadmap_alternatives_item_writes.py` passes -- [ ] 2.3 Keep the other alternatives tests green with the new order +- [x] 2.3 Keep the other alternatives tests green with the new order - Verification: `$ uv run devops ci test tests/roadmap/test_roadmap_alternatives.py` ## 3. The documents match the new order @@ -32,7 +32,7 @@ Kind: documents. Changes: the documents that state "plan all, then write" for the stage and alternatives rows, if any. -- [ ] 3.1 Fix each document that says the stage row or the alternatives row writes after it plans all items +- [x] 3.1 Fix each document that says the stage row or the alternatives row writes after it plans all items - Verification: `$ uv run devops ci docs` ## Criteria diff --git a/src/devops_cli/commands/roadmap.py b/src/devops_cli/commands/roadmap.py index e213a6a36..447fba6b4 100644 --- a/src/devops_cli/commands/roadmap.py +++ b/src/devops_cli/commands/roadmap.py @@ -305,15 +305,16 @@ def alternatives_cmd( return with _exit_on_failure("Could not check for alternatives"): _, store = _open_store(target) - plan = job.plan_alternatives(store, repo=target, item_number=item, limit=limit) + writer = job.AlternativesWriter(store) if confirm else None + plan = job.plan_alternatives( + store, repo=target, item_number=item, limit=limit, writer=writer + ) write_stdout(job.render_alternatives_plan(plan) + "\n") - if not confirm: + if writer is None: if plan.has_writes: print_info(MESSAGES.roadmap.preview_only) elif plan.has_writes: - with _exit_on_failure("Could not write the checks"): - written = job.apply_alternatives(store, plan) - print_success(MESSAGES.roadmap.alternatives_applied.format(count=written)) + print_success(MESSAGES.roadmap.alternatives_applied.format(count=writer.written)) with _exit_on_failure("The check is incomplete"): job.raise_for_failed_checks(plan) @@ -378,7 +379,7 @@ def stage_cmd( """Make Ready items Staged: a branch, an OpenSpec change in budgeted task groups, and a draft.""" _one_mode(dry_run=dry_run, plan=plan_only, confirm=confirm) from devops_cli.roadmap.stage.stage import ( - apply_stage, + StageWriter, dry_run_stage, emit_stage_results, parse_filters, @@ -403,6 +404,7 @@ def stage_cmd( with _exit_on_failure("Could not plan staging"): _, store = _open_store(target) clone = source or model_clone(target) + writer = StageWriter(store, clone) if confirm else None plan = plan_stage( store, repo=target, @@ -414,12 +416,12 @@ def stage_cmd( model=ToolboxModel(get_task_model("stage")), verifier=ToolboxModel(get_task_model("verification")), why=why, + writer=writer, ) write_stdout(render_stage_plan(plan) + "\n") failures = list(plan.failed) - if confirm: - with _exit_on_failure("Could not stage"): - applied = apply_stage(store, plan, source=clone) + if writer is not None: + applied = writer.applied() emit_stage_results(plan, applied) print_success( MESSAGES.roadmap.stage_applied.format( diff --git a/src/devops_cli/roadmap/alternatives.py b/src/devops_cli/roadmap/alternatives.py index 3e74f6983..7fe365c07 100644 --- a/src/devops_cli/roadmap/alternatives.py +++ b/src/devops_cli/roadmap/alternatives.py @@ -1088,8 +1088,11 @@ def plan_alternatives( limit: int | None = None, now: datetime | None = None, investigator: Any = None, + writer: AlternativesWriter | None = None, ) -> AlternativesPlan: - """Plans the checks of the selected items without writing anything. `limit` replaces both + """Plans the checks of the selected items. It writes nothing unless the caller gives a + `writer`: the writer then gets the result of each item as soon as that item is checked, so a + kill of the worker loses at most the check in progress (#4034). `limit` replaces both `roadmap.alternatives.limit` and `roadmap.alternatives.first_pass_limit`. `investigator` is the pydantic-ai model of the runs, the `analysis` task's by default.""" @@ -1121,6 +1124,8 @@ def plan_alternatives( failures: list[CheckFailure] = [] for item in selection.selected: outcome = _check_one(store, item, model=model, adrs=adrs, now=moment, fetch=web.fetch) + if writer is not None: + writer.settle(outcome) if isinstance(outcome, CheckFailure): failures.append(outcome) else: @@ -1158,42 +1163,56 @@ def _unchanged_since_read(store: RoadmapStore, check: Check) -> bool: return False -def apply_alternatives(store: RoadmapStore, plan: AlternativesPlan) -> int: - """Writes each check: the body block, the job record, then the comment, and counts it in - `devops_cli_roadmap_alternatives_total` by its verdict. It also observes the searches of the - check in `devops_cli_roadmap_alternatives_searches` by its verdict (#2950). Then records each failure in the job - record of its item: the time and the class of the error. Returns how many checks it wrote; a - check whose body a person changed since the read is left for the next run.""" - written = 0 - for check in plan.checks: - if not _unchanged_since_read(store, check): - continue - number = check.item.number - store.write_issue_body(number, check.new_body) - store.set_marks( - check.item, +class AlternativesWriter: + """The writes of one alternatives round, made when each item is checked (#4034). + + `plan_alternatives` gives it the result of each item. A kill of the worker then loses at most + the check in progress, and a write that a rate limit refuses ends the round with the checks + that are written. A check whose body a person changed since the read is left for the next + run. + """ + + def __init__(self, store: RoadmapStore) -> None: + self._store = store + self.written = 0 + + def settle(self, outcome: Check | CheckFailure) -> None: + """Write the result of one item. A check is counted in + `devops_cli_roadmap_alternatives_total` by its verdict, and its searches in + `devops_cli_roadmap_alternatives_searches` (#2950). A failure is recorded in the job record + of its item: the time and the class of the error.""" + if isinstance(outcome, CheckFailure): + self._store.set_marks( + outcome.item, + { + AlternativesRecordKey.FAILED_AT: outcome.failed_at.isoformat(), + AlternativesRecordKey.FAILED_ERROR: outcome.error_class, + }, + ) + return + if not _unchanged_since_read(self._store, outcome): + return + number = outcome.item.number + self._store.write_issue_body(number, outcome.new_body) + self._store.set_marks( + outcome.item, { - AlternativesRecordKey.CHECKED_AT: check.checked_at.isoformat(), - AlternativesRecordKey.BODY_HASH: check.body_hash, - AlternativesRecordKey.VERDICT: check.verdict.value, - AlternativesRecordKey.TOOL: check.tool.url if check.tool else None, + AlternativesRecordKey.CHECKED_AT: outcome.checked_at.isoformat(), + AlternativesRecordKey.BODY_HASH: outcome.body_hash, + AlternativesRecordKey.VERDICT: outcome.verdict.value, + AlternativesRecordKey.TOOL: outcome.tool.url if outcome.tool else None, AlternativesRecordKey.FAILED_AT: None, AlternativesRecordKey.FAILED_ERROR: None, }, ) - store.comment(number, check.comment) - emit(ROADMAP_ALTERNATIVES_TOTAL, 1, {"verdict": check.verdict.value}) - emit(ROADMAP_ALTERNATIVES_SEARCHES, len(check.searches), {"verdict": check.verdict.value}) - written += 1 - for failure in plan.failures: - store.set_marks( - failure.item, - { - AlternativesRecordKey.FAILED_AT: failure.failed_at.isoformat(), - AlternativesRecordKey.FAILED_ERROR: failure.error_class, - }, + self._store.comment(number, outcome.comment) + emit(ROADMAP_ALTERNATIVES_TOTAL, 1, {"verdict": outcome.verdict.value}) + emit( + ROADMAP_ALTERNATIVES_SEARCHES, + len(outcome.searches), + {"verdict": outcome.verdict.value}, ) - return written + self.written += 1 def raise_for_failed_checks(plan: AlternativesPlan) -> None: @@ -1266,6 +1285,7 @@ def render_alternatives_plan(plan: AlternativesPlan) -> str: "AlternativesPlan", "AlternativesReply", "AlternativesTools", + "AlternativesWriter", "Check", "CheckFailure", "CheckedTool", @@ -1274,7 +1294,6 @@ def render_alternatives_plan(plan: AlternativesPlan) -> str: "Selection", "ToolFacts", "Verdict", - "apply_alternatives", "body_hash_of", "dry_run_alternatives", "plan_alternatives", diff --git a/src/devops_cli/roadmap/run.py b/src/devops_cli/roadmap/run.py index e95f1372b..3b907b943 100644 --- a/src/devops_cli/roadmap/run.py +++ b/src/devops_cli/roadmap/run.py @@ -107,7 +107,7 @@ # `alternatives` imports pydantic_ai too: import it here for the same reason (#2535). from devops_cli.roadmap.alternatives import ( - apply_alternatives, + AlternativesWriter, plan_alternatives, raise_for_failed_checks, record_coverage, @@ -135,7 +135,7 @@ # `stage` imports the model client, which imports pydantic_ai too: import it here for the same # reason (#2659). from devops_cli.roadmap.stage.stage import ( - apply_stage, + StageWriter, emit_stage_results, parse_filters, plan_stage, @@ -590,8 +590,10 @@ def _run_stage_adapter( count each item by its result (#2659). An item that failed is recorded in its job record and waits for `roadmap.stage.retry_hours` hours; the job fails, once the others are written, only when it tried items and handled none (#3493). A failure that no item explains, such as an - outage, ends the run at once.""" + outage, ends the run at once. Each item is written when it is planned, so a stop of the + Service loses at most the item in progress (#4034).""" source = _job_clone("stage", clone_path) + writer = StageWriter(store, source) plan = plan_stage( store, repo=repo, @@ -599,8 +601,9 @@ def _run_stage_adapter( filters=parse_filters(()), model=ToolboxModel(get_task_model("stage")), verifier=ToolboxModel(get_task_model("verification")), + writer=writer, ) - applied = apply_stage(store, plan, source=source) + applied = writer.applied() emit_stage_results(plan, applied) synced = ( sync_statuses(store, repo=repo, planned=plan.planned) if plan.planned is not None else None @@ -681,15 +684,15 @@ def _run_alternatives_adapter(store: RoadmapStore, repo: str = "", **_: Any) -> `roadmap.alternatives.limit` open items, for an open tool that covers them (#2652). A failed check is recorded in its item's job record and the others are written; the job fails, naming the items, only when it tried items and checked none (#2949). A repository - the check is off for, such as a private one, costs a visibility read and nothing else. The - row sends the coverage gauges of the round before it writes, also when it writes nothing - (#2950).""" - plan = plan_alternatives(store, repo=repo) + the check is off for, such as a private one, costs a visibility read and nothing else. Each + check is written when it is done, so a kill of the worker loses at most the check in + progress, and a rate limit on a write ends the round with the checks that are written + (#4034). The row sends the coverage gauges of the round after the checks, also when it + writes nothing (#2950).""" + plan = plan_alternatives(store, repo=repo, writer=AlternativesWriter(store)) record_coverage(plan) if plan.off_reason: logger.info("The alternatives check of %s is off: %s", repo, plan.off_reason) - if plan.has_writes: - apply_alternatives(store, plan) raise_for_failed_checks(plan) return JobOutcome() diff --git a/src/devops_cli/roadmap/stage/stage.py b/src/devops_cli/roadmap/stage/stage.py index fa4612761..118b61f7c 100644 --- a/src/devops_cli/roadmap/stage/stage.py +++ b/src/devops_cli/roadmap/stage/stage.py @@ -74,7 +74,7 @@ import json import logging import shutil -from collections.abc import Callable, Iterable, Iterator, Mapping, Sequence +from collections.abc import Callable, Collection, Iterable, Iterator, Mapping, Sequence from contextlib import contextmanager from dataclasses import dataclass, field, replace from datetime import UTC, date, datetime, timedelta @@ -1269,26 +1269,32 @@ def _failure_of( def _plan_each( - planner: _Planner, targets: Iterable[StageTarget] + planner: _Planner, targets: Iterable[StageTarget], writer: StageWriter | None = None ) -> tuple[list[StagedChange], list[OverBudget], list[StageFailure], list[Blocked]]: + """Plan each target in order. With a `writer`, the result of a target is written as soon as the + target is done, before the next target is planned (#4034).""" ready: list[StagedChange] = [] stopped: list[OverBudget] = [] failed: list[StageFailure] = [] blocked: list[Blocked] = [] for target in targets: + result: StagedChange | OverBudget | Blocked | StageFailure try: - outcome = planner.plan(target) + result = planner.plan(target) except AICredentialsError: raise except (AIClientError, RoadmapStageError, ToolExecutionError) as exc: - failed.append(_failure_of(target, exc)) - continue - if isinstance(outcome, OverBudget): - stopped.append(outcome) - elif isinstance(outcome, Blocked): - blocked.append(outcome) + result = _failure_of(target, exc) + if writer is not None: + writer.settle(target, result) + if isinstance(result, StageFailure): + failed.append(result) + elif isinstance(result, OverBudget): + stopped.append(result) + elif isinstance(result, Blocked): + blocked.append(result) else: - ready.append(outcome) + ready.append(result) return ready, stopped, failed, blocked @@ -1442,9 +1448,14 @@ def plan_stage( verifier: ToolboxModel | None = None, why: bool = False, now: datetime | None = None, + writer: StageWriter | None = None, ) -> StagePlan: - """Plans the stage of up to `limit` items without a write to GitHub. When an item is selected, - the clone moves to the staged commit, the default branch of `origin` after the fetch (#3301). + """Plans the stage of up to `limit` items. It writes nothing unless the caller gives a + `writer`: the writer then gets the removals and the rejections of the items the round does not + select before the first plan call, and the result of each selected item as soon as that item + is done, so a stop of the Service loses at most the item in progress (#4034). When an item is + selected, the clone moves to the staged commit, the default branch of `origin` after the fetch + (#3301). `source` is the clone of the repository, at the default branch. `limit` and `step_tokens` override `roadmap.stage.limit` and `roadmap.stage.step_tokens`. At `roadmap.stage.planned_limit` @@ -1492,6 +1503,8 @@ def plan_stage( failed=tuple(_write_failure(f.item, f.error) for f in rejections.failures), rejections=rejections.found, ) + if writer is not None: + writer.begin(plan, {target.item.number for target in selection.selected}) if not selection.selected: return plan _move_clone(checkout, sha) @@ -1518,7 +1531,7 @@ def plan_stage( **reasons_of(store, (target.item for target in selection.selected)), }, ) - ready, stopped, failed, blocked = _plan_each(planner, selection.selected) + ready, stopped, failed, blocked = _plan_each(planner, selection.selected, writer) return replace( plan, ready=tuple(ready), @@ -1789,22 +1802,16 @@ def _moved_section(moved: Sequence[MovedPath]) -> str: return f"{texts.stage_comment_moved}{lines}" -def _moved_by_item(checkout: Path, plan: StagePlan) -> dict[int, tuple[MovedPath, ...]]: - """The named paths that the default branch changed after the base commit of `plan`, by the - number of each ready item (#3309). The default branch is fetched once, so the comparison holds - a commit that landed after the planner read the clone. A run with no ready change, or with no - base commit to compare, reads no git.""" - if not plan.ready or not plan.sha: - return {} +def _moved_for(checkout: Path, sha: str, change: StagedChange) -> tuple[MovedPath, ...]: + """The named paths of `change` that the default branch changed after the base commit `sha` + (#3309). The default branch is fetched for each item, just before its publish, so the + comparison holds a commit that landed during the plans of the earlier items. A run with no + base commit to compare reads no git.""" + if not sha: + return () default = default_branch(checkout, operation=_GIT_OPERATION) _git(checkout, "fetch", "-q", "origin", default) - branch = f"origin/{default}" - return { - change.target.item.number: tuple( - moved_paths(checkout, plan.sha, branch, _named_paths(change)) - ) - for change in plan.ready - } + return tuple(moved_paths(checkout, sha, f"origin/{default}", _named_paths(change))) def _notes(change: StagedChange, moved: Sequence[MovedPath] = ()) -> str: @@ -2043,81 +2050,125 @@ def _record_failures(store: RoadmapStore, failures: Sequence[StageFailure], now: ) -def apply_stage( - store: RoadmapStore, plan: StagePlan, *, source: Path | str, now: datetime | None = None -) -> StageApplied: - """Writes the plan: removes the staged branches of closed items, writes each rejected plan - (#3503), stages each ready change, and stops each item whose group is over the budget. A write that fails stops its item, and - the others go on; rejected GitHub credentials stop the run. Each item that failed, in the plan - or in a write, gets the time and the error class in its job record, and the job leaves it for - `roadmap.stage.retry_hours` hours (#3493). The stop of the Service stops the run too, but it - is no failed item: the failures before it are recorded, and the run ends with the stop - (#2753).""" - checkout = Path(source) - removed: list[int] = [] - rejected: list[int] = [] - returned: list[int] = [] - staged: list[int] = [] - stopped: list[int] = [] - failed: list[StageFailure] = [] - staging = {change.target.item.number for change in plan.ready} - moved = _moved_by_item(checkout, plan) - phases = ( - ( - [ - (r.item, partial(_remove_staged, store, plan.repo, checkout, r)) - for r in plan.removals - ], - removed, - ), - ( - [ - (r.item, partial(_reject_plan, store, checkout, r, staged=r.number in staging)) - for r in plan.rejections - ], - rejected, - ), - ( - [(b.target.item, partial(_return_blocked, store, b)) for b in plan.blocked], - returned, - ), - ( - [ - ( - c.target.item, - partial( - _stage_one, - store, - plan.repo, - checkout, - plan.sha, - c, - moved[c.target.item.number], - ), - ) - for c in plan.ready - ], - staged, - ), - ([(o.target.item, partial(_stop_on_budget, store, o)) for o in plan.over_budget], stopped), - ) - stopped_by: ServiceStoppingError | None = None - try: - for writes, done in phases: - write_each(writes, done, failed) - except ServiceStoppingError as stop: - stopped_by = stop - _record_failures(store, [*plan.failed, *failed], now or datetime.now(UTC)) - if stopped_by is not None: - raise stopped_by - return StageApplied( - tuple(staged), - tuple(stopped), - tuple(failed), - tuple(removed), - tuple(returned), - tuple(rejected), - ) +class StageWriter: + """The writes of one stage round, made when each item is done (#4034). + + `plan_stage` calls `begin` once, and `settle` for each selected item as soon as the item is + planned. A stop of the Service then ends the round and loses at most the item in progress. + `begin` removes the staged branches of closed items and writes the rejected plans of the items + the round does not select (#3503): these need no model. `settle` writes the rejection of a + selected item between its plan and its publish, then the result of the item, and reads the + moved paths of the item just before its publish. A write that fails stops its item, and the + others go on; rejected GitHub credentials stop the run. Each item that failed, in the plan or + in a write, gets the time and the error class in its job record, and the job leaves it for + `roadmap.stage.retry_hours` hours (#3493). The stop of the Service stops the run too, but it is + no failed item: the failures before it are recorded, and the run ends with the stop (#2753). + """ + + def __init__(self, store: RoadmapStore, source: Path | str, *, now: datetime | None = None): + self._store = store + self._source = Path(source) + self._now = now + self._repo = "" + self._sha = "" + self._rejections: dict[int, Rejection] = {} + self._removed: list[int] = [] + self._rejected: list[int] = [] + self._returned: list[int] = [] + self._staged: list[int] = [] + self._stopped: list[int] = [] + self._failed: list[StageFailure] = [] + + def applied(self) -> StageApplied: + """What the round wrote so far.""" + return StageApplied( + tuple(self._staged), + tuple(self._stopped), + tuple(self._failed), + tuple(self._removed), + tuple(self._returned), + tuple(self._rejected), + ) + + def begin(self, plan: StagePlan, selected: Collection[int]) -> None: + """Write what needs no model: the removals, the rejections of the items outside `selected` + 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} + with self._recording(plan.failed): + write_each( + [ + (r.item, partial(_remove_staged, self._store, plan.repo, self._source, r)) + for r in plan.removals + ], + self._removed, + self._failed, + ) + write_each( + [ + ( + r.item, + partial(_reject_plan, self._store, self._source, r, staged=False), + ) + for r in plan.rejections + if r.number not in selected + ], + self._rejected, + self._failed, + ) + + def settle( + self, target: StageTarget, result: StagedChange | OverBudget | Blocked | StageFailure + ) -> None: + """Write the result of one selected item: its rejection, if a person rejected its plan, + then the branch and the draft pull request of a change, the stop on the budget, the return + for open blockers, or the record of a failure.""" + with self._recording((result,) if isinstance(result, StageFailure) else ()): + self._settle(target.item, result) + + def _settle( + self, item: Item, result: StagedChange | OverBudget | Blocked | StageFailure + ) -> None: + store, source = self._store, self._source + if (rejection := self._rejections.pop(item.number, None)) is not None: + staged = isinstance(result, StagedChange) + write_each( + [(item, partial(_reject_plan, store, source, rejection, staged=staged))], + self._rejected, + self._failed, + ) + if isinstance(result, StagedChange): + write_each([(item, partial(self._stage, result))], self._staged, self._failed) + elif isinstance(result, OverBudget): + write_each( + [(item, partial(_stop_on_budget, store, result))], self._stopped, self._failed + ) + elif isinstance(result, Blocked): + write_each( + [(item, partial(_return_blocked, store, result))], self._returned, self._failed + ) + + 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) + + @contextmanager + def _recording(self, planned: Iterable[StageFailure]) -> Iterator[None]: + """Record the failures of the plan and the failures of the writes made in the block. The + stop of the Service leaves its own write undone, so the failures that came before are + recorded first and the stop goes on (#2753). Rejected credentials record nothing.""" + mark = len(self._failed) + try: + yield + except ServiceStoppingError: + self._record(planned, mark) + raise + self._record(planned, mark) + + def _record(self, planned: Iterable[StageFailure], mark: int) -> None: + _record_failures( + self._store, [*planned, *self._failed[mark:]], self._now or datetime.now(UTC) + ) class StageResult(StrEnum): @@ -2296,9 +2347,9 @@ def raise_for_failed_round(plan: StagePlan, applied: StageApplied) -> None: "StageResult", "StageSelection", "StageTarget", + "StageWriter", "StagedChange", "Walk", - "apply_stage", "dry_run_stage", "emit_stage_results", "named_items", diff --git a/tests/roadmap/stage/test_roadmap_stage_apply.py b/tests/roadmap/stage/test_roadmap_stage_apply.py index 4d9e18bb5..202e53b1c 100644 --- a/tests/roadmap/stage/test_roadmap_stage_apply.py +++ b/tests/roadmap/stage/test_roadmap_stage_apply.py @@ -22,7 +22,6 @@ StageFailure, StagePlan, StageResult, - apply_stage, parse_filters, plan_stage, render_stage_plan, @@ -40,6 +39,7 @@ VALID_REPORT, StageRepo, advance_origin, + apply_stage, heavy_group, seed_stageable, validate_args, diff --git a/tests/roadmap/stage/test_roadmap_stage_reconcile.py b/tests/roadmap/stage/test_roadmap_stage_reconcile.py index 15b156f69..7a87a8ae3 100644 --- a/tests/roadmap/stage/test_roadmap_stage_reconcile.py +++ b/tests/roadmap/stage/test_roadmap_stage_reconcile.py @@ -8,11 +8,11 @@ import pytest -from devops_cli.roadmap.stage.stage import StagePlan, apply_stage, parse_filters, plan_stage +from devops_cli.roadmap.stage.stage import StagePlan, parse_filters, plan_stage from devops_cli.roadmap.stores.store import GitHubState, ItemField, StageRecordKey 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, StageRepo +from tests.support.roadmap_stage import STAGE_NAME, STAGE_REPO, StageRepo, apply_stage if TYPE_CHECKING: from tests.support.fakes.github import GitHubApiFake diff --git a/tests/roadmap/stage/test_roadmap_stage_rejected.py b/tests/roadmap/stage/test_roadmap_stage_rejected.py index fcd4eb6bb..ca1cd1490 100644 --- a/tests/roadmap/stage/test_roadmap_stage_rejected.py +++ b/tests/roadmap/stage/test_roadmap_stage_rejected.py @@ -21,7 +21,6 @@ from devops_cli.roadmap.stage.stage import ( StagePlan, StageSelection, - apply_stage, parse_filters, plan_stage, read_stage_selection, @@ -41,6 +40,7 @@ STAGE_REPO, VALID_REPORT, StageRepo, + apply_stage, no_findings, seed_stageable, validate_args, diff --git a/tests/roadmap/stage/test_roadmap_stage_retry.py b/tests/roadmap/stage/test_roadmap_stage_retry.py index e2f4ca7dd..822fca1ed 100644 --- a/tests/roadmap/stage/test_roadmap_stage_retry.py +++ b/tests/roadmap/stage/test_roadmap_stage_retry.py @@ -15,7 +15,6 @@ StageApplied, StageFailure, StagePlan, - apply_stage, parse_filters, raise_for_failed_round, select_stageable, @@ -23,7 +22,7 @@ from devops_cli.roadmap.stores.store import ItemField, StageRecordKey from tests.support.fakes.roadmap_store import InMemoryRoadmapStore from tests.support.roadmap_implement import JobWorld -from tests.support.roadmap_stage import STAGE_REPO, seed_stageable +from tests.support.roadmap_stage import STAGE_REPO, apply_stage, seed_stageable pytestmark = pytest.mark.usefixtures("keyring_store") diff --git a/tests/roadmap/stage/test_roadmap_stage_verification.py b/tests/roadmap/stage/test_roadmap_stage_verification.py index 52ea93003..867a18f65 100644 --- a/tests/roadmap/stage/test_roadmap_stage_verification.py +++ b/tests/roadmap/stage/test_roadmap_stage_verification.py @@ -13,7 +13,6 @@ StagePlan, _label_needs_fix, _notes, - apply_stage, parse_filters, plan_stage, ) @@ -28,6 +27,7 @@ STAGE_REPO, VALID_REPORT, StageRepo, + apply_stage, seed_stageable, validate_args, ) diff --git a/tests/roadmap/test_roadmap_alternatives.py b/tests/roadmap/test_roadmap_alternatives.py index d5831e7c6..e64a0ef5c 100644 --- a/tests/roadmap/test_roadmap_alternatives.py +++ b/tests/roadmap/test_roadmap_alternatives.py @@ -42,7 +42,6 @@ AlternativesReply, AlternativesTools, Verdict, - apply_alternatives, body_hash_of, plan_alternatives, raise_for_failed_checks, @@ -74,6 +73,7 @@ from tests.support.fakes.http import WebFake from tests.support.fakes.otlp import FakeCollector from tests.support.fakes.roadmap_store import InMemoryRoadmapStore +from tests.support.roadmap_alternatives import apply_alternatives from tests.support.scripted_model import Script, call pytestmark = pytest.mark.usefixtures("keyring_store") diff --git a/tests/support/roadmap_alternatives.py b/tests/support/roadmap_alternatives.py new file mode 100644 index 000000000..baa46f162 --- /dev/null +++ b/tests/support/roadmap_alternatives.py @@ -0,0 +1,23 @@ +"""The writes of a finished alternatives plan, for the tests of the check (#4034).""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +from devops_cli.roadmap.alternatives import AlternativesPlan, AlternativesWriter + +if TYPE_CHECKING: + from devops_cli.roadmap.stores.store import RoadmapStore + + +def apply_alternatives(store: RoadmapStore, plan: AlternativesPlan) -> int: + """Write the checks and the failures of a finished `plan` with the writer of the job, and + return how many checks were written. A plan that `plan_alternatives` made without a writer is + written the way a round writes it, so a test can look at the plan before it looks at the + writes.""" + writer = AlternativesWriter(store) + for check in plan.checks: + writer.settle(check) + for failure in plan.failures: + writer.settle(failure) + return writer.written diff --git a/tests/support/roadmap_stage.py b/tests/support/roadmap_stage.py index dae011da0..257f2cf7e 100644 --- a/tests/support/roadmap_stage.py +++ b/tests/support/roadmap_stage.py @@ -12,11 +12,17 @@ import subprocess from collections.abc import Mapping from dataclasses import dataclass +from datetime import datetime from pathlib import Path from typing import TYPE_CHECKING import pytest +from devops_cli.roadmap.stage.stage import ( + StageApplied, + StagePlan, + StageWriter, +) from devops_cli.roadmap.stage.stage_change import StageGroup from devops_cli.roadmap.stage.stage_verify import Verification from devops_cli.roadmap.stores.store import ItemField @@ -259,3 +265,26 @@ def advance_origin(repo: StageRepo, root: Path, files: Mapping[str, str]) -> Non "advance main", ) run_git(other, "push", "-q", "origin", "main") + + +def apply_stage( + store: InMemoryRoadmapStore, plan: StagePlan, *, source: Path | str, now: datetime | None = None +) -> StageApplied: + """Write a finished `plan` with the writer of the job: the removals and the rejections of the + items that the plan does not stage first, then the result of each item. A plan that + `plan_stage` made without a writer is written the way a round writes it, so a test can look at + the plan before it looks at the writes.""" + writer = StageWriter(store, source, now=now) + selected = { + *(b.target.item.number for b in plan.blocked), + *(c.target.item.number for c in plan.ready), + *(o.target.item.number for o in plan.over_budget), + } + writer.begin(plan, selected) + for blocked in plan.blocked: + writer.settle(blocked.target, blocked) + for change in plan.ready: + writer.settle(change.target, change) + for stopped in plan.over_budget: + writer.settle(stopped.target, stopped) + return writer.applied() From 9f9bab55697f3eb6d0f1de742f797d2013c1de59 Mon Sep 17 00:00:00 2001 From: Daniel Petty Date: Sat, 7 Nov 2026 21:43:45 +0000 Subject: [PATCH 4/5] docs(openspec): archive 4034 write each stage and alternatives item --- .../.openspec.yaml | 0 .../design.md | 0 .../proposal.md | 0 .../specs/roadmap-alternatives/spec.md | 0 .../specs/roadmap-stage/spec.md | 0 .../tasks.md | 0 openspec/specs/roadmap-alternatives/spec.md | 21 +++++++++++ openspec/specs/roadmap-stage/spec.md | 36 +++++++++++++++++++ 8 files changed, 57 insertions(+) rename openspec/changes/{4034-write-each-stage-alternatives-item => archive/2026-11-07-4034-write-each-stage-alternatives-item}/.openspec.yaml (100%) rename openspec/changes/{4034-write-each-stage-alternatives-item => archive/2026-11-07-4034-write-each-stage-alternatives-item}/design.md (100%) rename openspec/changes/{4034-write-each-stage-alternatives-item => archive/2026-11-07-4034-write-each-stage-alternatives-item}/proposal.md (100%) rename openspec/changes/{4034-write-each-stage-alternatives-item => archive/2026-11-07-4034-write-each-stage-alternatives-item}/specs/roadmap-alternatives/spec.md (100%) rename openspec/changes/{4034-write-each-stage-alternatives-item => archive/2026-11-07-4034-write-each-stage-alternatives-item}/specs/roadmap-stage/spec.md (100%) rename openspec/changes/{4034-write-each-stage-alternatives-item => archive/2026-11-07-4034-write-each-stage-alternatives-item}/tasks.md (100%) diff --git a/openspec/changes/4034-write-each-stage-alternatives-item/.openspec.yaml b/openspec/changes/archive/2026-11-07-4034-write-each-stage-alternatives-item/.openspec.yaml similarity index 100% rename from openspec/changes/4034-write-each-stage-alternatives-item/.openspec.yaml rename to openspec/changes/archive/2026-11-07-4034-write-each-stage-alternatives-item/.openspec.yaml diff --git a/openspec/changes/4034-write-each-stage-alternatives-item/design.md b/openspec/changes/archive/2026-11-07-4034-write-each-stage-alternatives-item/design.md similarity index 100% rename from openspec/changes/4034-write-each-stage-alternatives-item/design.md rename to openspec/changes/archive/2026-11-07-4034-write-each-stage-alternatives-item/design.md diff --git a/openspec/changes/4034-write-each-stage-alternatives-item/proposal.md b/openspec/changes/archive/2026-11-07-4034-write-each-stage-alternatives-item/proposal.md similarity index 100% rename from openspec/changes/4034-write-each-stage-alternatives-item/proposal.md rename to openspec/changes/archive/2026-11-07-4034-write-each-stage-alternatives-item/proposal.md diff --git a/openspec/changes/4034-write-each-stage-alternatives-item/specs/roadmap-alternatives/spec.md b/openspec/changes/archive/2026-11-07-4034-write-each-stage-alternatives-item/specs/roadmap-alternatives/spec.md similarity index 100% rename from openspec/changes/4034-write-each-stage-alternatives-item/specs/roadmap-alternatives/spec.md rename to openspec/changes/archive/2026-11-07-4034-write-each-stage-alternatives-item/specs/roadmap-alternatives/spec.md diff --git a/openspec/changes/4034-write-each-stage-alternatives-item/specs/roadmap-stage/spec.md b/openspec/changes/archive/2026-11-07-4034-write-each-stage-alternatives-item/specs/roadmap-stage/spec.md similarity index 100% rename from openspec/changes/4034-write-each-stage-alternatives-item/specs/roadmap-stage/spec.md rename to openspec/changes/archive/2026-11-07-4034-write-each-stage-alternatives-item/specs/roadmap-stage/spec.md diff --git a/openspec/changes/4034-write-each-stage-alternatives-item/tasks.md b/openspec/changes/archive/2026-11-07-4034-write-each-stage-alternatives-item/tasks.md similarity index 100% rename from openspec/changes/4034-write-each-stage-alternatives-item/tasks.md rename to openspec/changes/archive/2026-11-07-4034-write-each-stage-alternatives-item/tasks.md diff --git a/openspec/specs/roadmap-alternatives/spec.md b/openspec/specs/roadmap-alternatives/spec.md index 6bd76b929..fce21bc30 100644 --- a/openspec/specs/roadmap-alternatives/spec.md +++ b/openspec/specs/roadmap-alternatives/spec.md @@ -574,3 +574,24 @@ The check SHALL be off when the repository is private, when its visibility canno #### Scenario: No search provider - **WHEN** the repository is public and no search provider is configured - **THEN** the plan says that no search provider is configured, and no search runs + +### Requirement: An alternatives round writes each check when the check is done +An alternatives round SHALL write the body block, the record and the comment of check N before check N+1 starts. A kill of the worker SHALL lose at most the check in progress. + +#### Scenario: Kill in item 3 of 5 +- **WHEN** a round selects 5 items and the model call of the third item raises a `BaseException` that no handler catches +- **THEN** checks 1 and 2 have their body block, record and comment, and the other items have none + +### Requirement: A rate limit ends an alternatives round and keeps the written checks +An alternatives round SHALL end when a write fails with a GitHub rate limit. It SHALL keep the checks that it wrote and SHALL NOT check the items after the failed one. + +#### Scenario: Rate limit in check 2 +- **WHEN** the write of the body of item 2 of 5 fails with a rate-limit error +- **THEN** the round raises the error, item 1 keeps its written check, and the model gets no request for items 3, 4 and 5 + +### Requirement: An alternatives plan writes nothing +`devops roadmap alternatives --plan` SHALL make no write to the roadmap store. + +#### Scenario: Plan only +- **WHEN** the command runs with `--plan` on 3 items that have no check +- **THEN** the store receives no write diff --git a/openspec/specs/roadmap-stage/spec.md b/openspec/specs/roadmap-stage/spec.md index eb939f3e9..ccd8fafb6 100644 --- a/openspec/specs/roadmap-stage/spec.md +++ b/openspec/specs/roadmap-stage/spec.md @@ -2787,3 +2787,39 @@ The plan stage prompt SHALL tell the model to cite the staged value of each cons #### Scenario: Prompt rule - **WHEN** The prompt rules test reads `src/devops_cli/ai/tasks/roadmap_stage_change.md`. - **THEN** The text holds an instruction to cite the staged value of each constant that the plan relies on. + +### Requirement: A stage round writes each item when the item is done +A stage round SHALL plan one selected item, write its result, and only then plan the next item. The result is the publish of a ready change, the return of a blocked item, or the stop comment of an item over the budget. A kill of the worker SHALL lose at most the item in progress. + +#### Scenario: Kill between item 1 and item 2 +- **WHEN** a round selects 3 items and the model call of item 2 raises a `BaseException` that no handler catches +- **THEN** item 1 has its pushed branch, its draft pull request, its record and its comment, and items 2 and 3 have no branch, no record and no comment + +### Requirement: A stop ends a stage round after it records the finished items +A stop of the Service or rejected GitHub credentials SHALL end a stage round. The round SHALL record the failure of each item that failed before the stop, and SHALL keep the written result of each finished item. Any other error SHALL be the failure of one item, and the round SHALL go on. + +#### Scenario: Stop of the Service in item 3 +- **WHEN** item 1 is staged, the plan of item 2 fails, and the Service stops in the plan of item 3 of 4 items +- **THEN** item 1 stays staged, item 2 has a failure record, items 3 and 4 have no record and no comment, and the round ends with the stop + +### Requirement: A stage round writes the unselected items first +A stage round SHALL write the removals of closed items and the rejections of items that it does not select before its first plan call. A rejection of a selected item SHALL be written after the plan of that item and before its publish. The round SHALL read the moved paths of an item just before it publishes that item. + +#### Scenario: Removal and rejection of unselected items +- **WHEN** a round has a closed item with a staged branch, an item with a rejected plan that the round leaves out, and one item to plan +- **THEN** the removal and the rejection are written before the first plan call + +#### Scenario: Rejection of a selected item +- **WHEN** a person names an item whose plan pull request was closed with no merge +- **THEN** the rejection is written after the plan of the item and before its publish, the staged branch of the rejected plan is deleted before the new push, and the Status of the item stays Ready + +#### Scenario: A commit lands during the plan of item 2 +- **WHEN** a commit that changes a path named by both plans lands on the default branch during the plan of item 2 +- **THEN** the comment of item 1 lists no moved path and the comment of item 2 lists the path + +### Requirement: A stage plan writes nothing +`devops roadmap stage --plan` SHALL make no write to GitHub or to the remote repository, also when the round holds a removal or a rejection. + +#### Scenario: Plan only +- **WHEN** the command runs with `--plan` on a roadmap that holds a removal, a rejection and a ready item +- **THEN** the store receives no write, no branch is pushed or deleted, and no GitHub write request is sent From c31a450c299ad39555dde8af9a3313fe796e2850 Mon Sep 17 00:00:00 2001 From: Daniel Petty Date: Sat, 7 Nov 2026 21:59:01 +0000 Subject: [PATCH 5/5] test(roadmap): load the writer helpers against a base tree --- tests/support/roadmap_alternatives.py | 6 +++++- tests/support/roadmap_stage.py | 10 +++++----- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/tests/support/roadmap_alternatives.py b/tests/support/roadmap_alternatives.py index baa46f162..9312a5ce8 100644 --- a/tests/support/roadmap_alternatives.py +++ b/tests/support/roadmap_alternatives.py @@ -4,7 +4,7 @@ from typing import TYPE_CHECKING -from devops_cli.roadmap.alternatives import AlternativesPlan, AlternativesWriter +from devops_cli.roadmap.alternatives import AlternativesPlan if TYPE_CHECKING: from devops_cli.roadmap.stores.store import RoadmapStore @@ -15,6 +15,10 @@ def apply_alternatives(store: RoadmapStore, plan: AlternativesPlan) -> int: return how many checks were written. A plan that `plan_alternatives` made without a writer is written the way a round writes it, so a test can look at the plan before it looks at the writes.""" + # Imported here, so that the fail-to-pass replay loads this module against a base tree that + # has no `AlternativesWriter` yet. + from devops_cli.roadmap.alternatives import AlternativesWriter + writer = AlternativesWriter(store) for check in plan.checks: writer.settle(check) diff --git a/tests/support/roadmap_stage.py b/tests/support/roadmap_stage.py index 257f2cf7e..03ed6190f 100644 --- a/tests/support/roadmap_stage.py +++ b/tests/support/roadmap_stage.py @@ -18,11 +18,7 @@ import pytest -from devops_cli.roadmap.stage.stage import ( - StageApplied, - StagePlan, - StageWriter, -) +from devops_cli.roadmap.stage.stage import StageApplied, StagePlan from devops_cli.roadmap.stage.stage_change import StageGroup from devops_cli.roadmap.stage.stage_verify import Verification from devops_cli.roadmap.stores.store import ItemField @@ -274,6 +270,10 @@ def apply_stage( items that the plan does not stage first, then the result of each item. A plan that `plan_stage` made without a writer is written the way a round writes it, so a test can look at the plan before it looks at the writes.""" + # Imported here: this module loads as a pytest plugin, and the fail-to-pass replay loads it + # against a base tree that has no `StageWriter` yet. + from devops_cli.roadmap.stage.stage import StageWriter + writer = StageWriter(store, source, now=now) selected = { *(b.target.item.number for b in plan.blocked),