diff --git a/docs/ROADMAP_WORKFLOW.md b/docs/ROADMAP_WORKFLOW.md index 5b8d6bfc6..d4bec4746 100644 --- a/docs/ROADMAP_WORKFLOW.md +++ b/docs/ROADMAP_WORKFLOW.md @@ -180,7 +180,7 @@ A stop has a class (`roadmap/failure.py`, #3065). A stop site that knows its cau The Job runs these actions by itself: - the fix rounds of `roadmap.implement.repair_rounds` for a failed check, - `amend` for a request of the kinds `path`, `command` and `task`, -- `return` for a `premise` request, +- `return` for a `premise` request, and for a behavior group that adds a guard which the branch already defines ([the premise of a group](roadmap/implement.md)), - the split of a group whose inputs exceed the context window. A plan can order the removal or the rename of a test (#3490). Its `proposal.md` names the old test as `path::name` (`path::Class.test_x` for a test of a class) in the `check` of an entry of `## Weakened checks`, and the entry says what the test stopped and what safeguard takes its place: `- ****: Stopped: Safeguard: `. The step guard allows that test. It refuses any other lost test, with the class `tests_lost`. AGENTS.md rule 5 lets only the owner lower a threshold, and only a person merges a pull request, so the Job reads the record from `origin/` and never from its checkout. An entry that the agent writes waives nothing, and a plan that is only on its staged branch records nothing. A step that a person runs with `devops roadmap implement --item --group ` reads no record, so it refuses each lost test. The plan reader refuses a line of the section in another form. diff --git a/docs/roadmap/implement.md b/docs/roadmap/implement.md index ffd597236..4e1787232 100644 --- a/docs/roadmap/implement.md +++ b/docs/roadmap/implement.md @@ -75,6 +75,8 @@ The job never merges. [Checks of the implement job](../ROADMAP_WORKFLOW.md#check **A stop.** A step that fails stops the item. The job comments why, shows a table of the steps (the step, its attempts, its tokens in and out, and its last failing command), shows the masked diff of the discarded work, and sets the Status to Blocked. The job keeps the trace of its run in `implement.trace`: each model call, tool call, check and retry, with no argument, no output, no secret and no private host name. Read it with `devops roadmap explain --trace`. The job keeps no Job artifact. The Service runs the item again after the comment `implement: retry`. For a stop whose class a new attempt can fix, it runs the item again with no comment. This happens after a Job that ended with no result, up to `roadmap.implement.continuations_per_item` times (default 2). It also happens after a new runner image, within the limits of `roadmap.implement.digest_clears_per_class` and `roadmap.implement.digest_clears_per_item`. +**The premise of a behavior group.** Before a behavior group starts, the job checks its premise at the branch tip. It reads each task of the group that has an add verb, the word guard or check, and one name in a code span. When the branch tip defines that name, the job returns the item to New. The comment holds the task and the definition line. The job runs no step, makes no model request and writes nothing. The check uses the same definition lookup as the premise check of the stage job (`src/devops_cli/roadmap/premise.py`). It reads the last commit that is not a `wip:` commit, so a guard that an earlier attempt of the same group kept is no defect. A group of another kind gets no check. + **A plan request.** A stopped step can request a bounded change of its plan in a `plan-request` block. The job judges requests of the kinds `path`, `command` and `task` with deterministic rules. The job commits an accepted amendment, records it in `tasks.md`, and runs the group again. A group whose inputs exceed the limit is split once for each item. A `premise` request returns the item to New with its evidence. Other requests stop the item as a question for a person. **Claims.** The job leaves out an item that an agent or a person claimed with the label `owner/agent` or `owner/human`. When one claims an item that a running job works on, the job ends it at the next group boundary, with no push and no pull request. diff --git a/openspec/changes/3417-the-implement-job-re-checks-a/.openspec.yaml b/openspec/changes/archive/2026-11-06-3417-the-implement-job-re-checks-a/.openspec.yaml similarity index 100% rename from openspec/changes/3417-the-implement-job-re-checks-a/.openspec.yaml rename to openspec/changes/archive/2026-11-06-3417-the-implement-job-re-checks-a/.openspec.yaml diff --git a/openspec/changes/3417-the-implement-job-re-checks-a/design.md b/openspec/changes/archive/2026-11-06-3417-the-implement-job-re-checks-a/design.md similarity index 68% rename from openspec/changes/3417-the-implement-job-re-checks-a/design.md rename to openspec/changes/archive/2026-11-06-3417-the-implement-job-re-checks-a/design.md index e09cd26b5..f6e487b09 100644 --- a/openspec/changes/3417-the-implement-job-re-checks-a/design.md +++ b/openspec/changes/archive/2026-11-06-3417-the-implement-job-re-checks-a/design.md @@ -5,7 +5,7 @@ - **Put the shared definition check in the new module `src/devops_cli/roadmap/premise.py`, and import it from the stage probe and the implement job.**: A private import from `stage_probe.py` would couple the implement job to the internals of the stage probe, and a copy would be two checks to keep in step. - **Read the branch tip of the worktree as the commit of the check.**: The branch tip holds the staged plan and the work of the earlier groups, so the check also catches a guard that an earlier group adds. - **Detect a task that adds a guard from its text: an add verb, a word such as guard or check, and one backticked identifier.**: The rule is deterministic and needs no model call, so the test asserts it offline with a fixed tasks file. -- **Read the kind of a group from the public `group_kind` of `src/devops_cli/roadmap/plan_read.py`, and run the check for a behavior group only.**: `TaskGroup` carries no kind. The plan reader already infers the kind from a `Kind:` line or the `fails on the base` suffix of the first task, so the implement job reuses that inference. Only a behavior group adds a product guard; a document or test group would return an item for a name it never adds. +- **Read the kind of a group from `TaskGroup.is_behavior` of `src/devops_cli/roadmap/step_files.py`, and run the check for a behavior group only.**: The property applies the rule of the plan reader: a `Kind:` line or the `fails on the base` suffix of the first task. The implement step already uses it. Only a behavior group adds a product guard; a document or test group would return an item for a name it never adds. - **Return the item to New with the definition line and run no group step.**: CONTEXT.md and the failure table of `docs/ROADMAP_WORKFLOW.md` record that a false premise returns the item to New with its evidence. ## Rejected alternatives @@ -13,4 +13,4 @@ - **Keep the premise check inside the step model and trust it to notice the defect.**: The issue shows the plan of PR #3374 for #3368 passed `check_proposal` and would pass the plan check of #3393, so only a deterministic check at the branch tip catches the duplicate guard. - **Copy the definition lookup into the implement job and keep the stage probe as it is.**: `src/devops_cli/roadmap/stage_probe.py` already holds `_definition_pattern` and the definition branch of `_Run.identifier`; a copy makes two checks to keep in step, which the issue rejects. - **Run the premise check for every group, whatever its kind.**: A group whose kind is not behavior adds no product guard, so the check would return an item for a document or test task that names an existing helper. -- **Add a `kind` field to `TaskGroup`, or read the kind with the private `plan_read._kind`.**: The writer drops the kind of a group from `tasks.md`, and the plan reader infers it. A second inference in `step_files.py` would duplicate the first, and `step_files.py` cannot import `stage_change.py` without a cycle. A private import would couple the implement job to the reader internals. +- **Add a second kind inference to the implement job, or read the private `plan_read._kind`.**: `TaskGroup.is_behavior` already gives the kind to the implement job. A second inference would duplicate it, and a private import would couple the implement job to the reader internals. diff --git a/openspec/changes/3417-the-implement-job-re-checks-a/proposal.md b/openspec/changes/archive/2026-11-06-3417-the-implement-job-re-checks-a/proposal.md similarity index 83% rename from openspec/changes/3417-the-implement-job-re-checks-a/proposal.md rename to openspec/changes/archive/2026-11-06-3417-the-implement-job-re-checks-a/proposal.md index 4c15c9cf1..ba0a83e23 100644 --- a/openspec/changes/3417-the-implement-job-re-checks-a/proposal.md +++ b/openspec/changes/archive/2026-11-06-3417-the-implement-job-re-checks-a/proposal.md @@ -8,11 +8,11 @@ The implement job runs the task groups of a Staged plan without a deterministic - Extract the definition lookup of the stage probe into one shared function in the new module `src/devops_cli/roadmap/premise.py`. - Call the shared function from `src/devops_cli/roadmap/stage_probe.py` so the stage job and the implement job share one definition check. -- Expose the group-kind inference that the plan reader already uses as `group_kind` in `src/devops_cli/roadmap/plan_read.py`. +- Read the kind of a group from `TaskGroup.is_behavior` in `src/devops_cli/roadmap/step_files.py`, which the implement step already uses. - Check the premise of each behavior group at the branch tip in `src/devops_cli/roadmap/implement_run.py`, before the group step runs. - Return the item to New with the definition line when a behavior group task adds a guard that the branch tip defines, and run no step and write no file. - Add the false-premise and true-premise tests to `tests/roadmap/test_roadmap_implement_job.py`. -- Record the new check on the implement page `docs/roadmap/implement.md`. +- Record the new check on the implement page `docs/roadmap/implement.md` and in `docs/ROADMAP_WORKFLOW.md`. - Add the shared check and the behavior-group premise check as requirements of the capability `roadmap-implement-job`, so the delta holds the new behavior. - The change fits one pull request. @@ -28,8 +28,8 @@ None. ## Impact -- Code: `src/devops_cli/roadmap/premise.py`, `src/devops_cli/roadmap/stage_probe.py`, `src/devops_cli/roadmap/plan_read.py`, `src/devops_cli/roadmap/implement_run.py` -- Documents: `docs/roadmap/implement.md` +- Code: `src/devops_cli/roadmap/premise.py`, `src/devops_cli/roadmap/stage_probe.py`, `src/devops_cli/roadmap/implement_run.py`, `src/devops_cli/lang/en/messages.py` +- Documents: `docs/roadmap/implement.md`, `docs/ROADMAP_WORKFLOW.md` - Tests: `tests/roadmap/test_premise.py`, `tests/roadmap/test_roadmap_implement_job.py` ## Feasibility @@ -40,8 +40,8 @@ None. - The probes say of the shared module that this change adds: "The path `src/devops_cli/roadmap/premise.py` is no file of the staging commit. No commit in the history deleted or renamed it." So the module is a path to create, and the premise of the issue holds. - read_file `src/devops_cli/roadmap/stage_probe.py` showed the definition lookup that the change extracts: `_definition_pattern` builds the expression and `_Run.identifier` runs `git grep -n -E` with it at the staging commit. - read_file `src/devops_cli/roadmap/implement_run.py` showed the group loop and `_return_premise`, which runs only after the step model reports a premise request, so no deterministic check runs before a group today. -- read_file `src/devops_cli/roadmap/step_files.py` showed that `TaskGroup` holds the tasks and the text of a group, and no kind. So the implement job cannot read the kind from the parsed group. -- read_file `src/devops_cli/roadmap/plan_read.py` showed that `_kind` infers the kind of a group from a `Kind:` line or the `fails on the base` suffix of its first task, and that `_group` already uses the inference. The change exposes it as `group_kind`, so the implement job runs the check for a behavior group only. +- read_file `src/devops_cli/roadmap/step_files.py` showed that `TaskGroup.is_behavior` judges the kind of a group from a `Kind:` line or the `fails on the base` suffix of its first task, the same rule as the plan reader. The implement step already uses it (`implement_step.py`). +- read_file `src/devops_cli/roadmap/plan_read.py` showed that `_kind` applies the same rule for the plan check. So the implement job needs no new inference, and it runs the check for a behavior group only. - First checks against the real system: `uv run devops ci test tests/roadmap/test_premise.py tests/roadmap/test_roadmap_stage_probe.py`, then `uv run devops ci test tests/roadmap/test_roadmap_implement_job.py`, then `uv run devops ci docs`. ## Existing Tool Considered diff --git a/openspec/changes/3417-the-implement-job-re-checks-a/specs/roadmap-implement-job/spec.md b/openspec/changes/archive/2026-11-06-3417-the-implement-job-re-checks-a/specs/roadmap-implement-job/spec.md similarity index 95% rename from openspec/changes/3417-the-implement-job-re-checks-a/specs/roadmap-implement-job/spec.md rename to openspec/changes/archive/2026-11-06-3417-the-implement-job-re-checks-a/specs/roadmap-implement-job/spec.md index 124e852f1..38b178414 100644 --- a/openspec/changes/3417-the-implement-job-re-checks-a/specs/roadmap-implement-job/spec.md +++ b/openspec/changes/archive/2026-11-06-3417-the-implement-job-re-checks-a/specs/roadmap-implement-job/spec.md @@ -1,7 +1,7 @@ ## ADDED Requirements ### Requirement: The job checks the premise of a behavior group at the branch tip -Before the job runs a behavior group, the job SHALL check each guard or check identifier that a task of the group adds against the branch tip, and SHALL return the item to New with the definition line when the branch tip defines it. The job SHALL read the kind of the group from `group_kind` of `src/devops_cli/roadmap/plan_read.py`. +Before the job runs a behavior group, the job SHALL check each guard or check identifier that a task of the group adds against the branch tip, and SHALL return the item to New with the definition line when the branch tip defines it. The job SHALL read the kind of the group from `TaskGroup.is_behavior` of `src/devops_cli/roadmap/step_files.py`. #### Scenario: A task adds a guard that the base already runs - **WHEN** A behavior group task says to add the guard `_delivery_title` and the branch tip defines `_delivery_title` diff --git a/openspec/changes/3417-the-implement-job-re-checks-a/tasks.md b/openspec/changes/archive/2026-11-06-3417-the-implement-job-re-checks-a/tasks.md similarity index 54% rename from openspec/changes/3417-the-implement-job-re-checks-a/tasks.md rename to openspec/changes/archive/2026-11-06-3417-the-implement-job-re-checks-a/tasks.md index e2d70fd6b..c367cc354 100644 --- a/openspec/changes/3417-the-implement-job-re-checks-a/tasks.md +++ b/openspec/changes/archive/2026-11-06-3417-the-implement-job-re-checks-a/tasks.md @@ -4,25 +4,25 @@ Reads: `src/devops_cli/roadmap/stage_probe.py`, `src/devops_cli/roadmap/refine.py`, `tests/roadmap/test_roadmap_stage_probe.py`. Changes: `src/devops_cli/roadmap/premise.py` (new), `src/devops_cli/roadmap/stage_probe.py`, `tests/roadmap/test_premise.py` (new). -- [ ] 1.1 In `tests/roadmap/test_premise.py`, write `test_the_definition_lines_of_an_identifier_come_from_the_commit`, which runs the shared function on the commit of the checkout and asserts that the definition line of `_delivery_title` names `src/devops_cli/roadmap/implement_run.py`. Run it first and see it fail on the missing module +- [x] 1.1 In `tests/roadmap/test_premise.py`, write `test_the_definition_lines_of_an_identifier_come_from_the_commit`, which runs the shared function on the commit of the checkout and asserts that the definition line of `_delivery_title` names `src/devops_cli/roadmap/implement_run.py`. Run it first and see it fail on the missing module - Verification: `uv run devops ci test tests/roadmap/test_premise.py tests/roadmap/test_roadmap_stage_probe.py` fails on the base -- [ ] 1.2 Create `src/devops_cli/roadmap/premise.py` with `definition_lines(source, sha, name, git=git_result)`, which runs the extended `git grep` of `_definition_pattern` at `sha` and returns the definition lines, and `defines(source, sha, name)`, which reports whether the list is not empty. Move `_definition_pattern` and the definition branch of `_Run.identifier` of `src/devops_cli/roadmap/stage_probe.py` into the new module. Call `definition_lines` from `_Run.identifier` with `self.git`, so the stage probe keeps its command budget, its messages and its line limit +- [x] 1.2 Create `src/devops_cli/roadmap/premise.py` with `definition_lines(source, sha, name, git=git_result)`, which runs the extended `git grep` of `_definition_pattern` at `sha` and returns the definition lines, and `defines(source, sha, name)`, which reports whether the list is not empty. Move `_definition_pattern` and the definition branch of `_Run.identifier` of `src/devops_cli/roadmap/stage_probe.py` into the new module. Call `definition_lines` from `_Run.identifier` with `self.git`, so the stage probe keeps its command budget, its messages and its line limit - Verification: `uv run devops ci test tests/roadmap/test_premise.py tests/roadmap/test_roadmap_stage_probe.py` passes ## 2. Check the premise of a behavior group before its step -Reads: `src/devops_cli/roadmap/implement_run.py`, `src/devops_cli/roadmap/premise.py`, `src/devops_cli/roadmap/plan_read.py`, `src/devops_cli/roadmap/step_files.py`, `tests/roadmap/test_roadmap_implement_job.py`, `tests/support/roadmap_implement.py`. Changes: `src/devops_cli/roadmap/implement_run.py`, `src/devops_cli/roadmap/plan_read.py`, `tests/roadmap/test_roadmap_implement_job.py`. +Reads: `src/devops_cli/roadmap/implement_run.py`, `src/devops_cli/roadmap/premise.py`, `src/devops_cli/roadmap/step_files.py`, `tests/roadmap/test_roadmap_implement_job.py`, `tests/support/roadmap_implement.py`. Changes: `src/devops_cli/roadmap/implement_run.py`, `src/devops_cli/roadmap/premise.py`, `src/devops_cli/lang/en/messages.py`, `tests/roadmap/test_premise.py`, `tests/roadmap/test_roadmap_implement_job.py`. -- [ ] 2.1 In `tests/roadmap/test_roadmap_implement_job.py`, add `test_a_behavior_group_that_adds_a_guard_the_base_already_runs_returns_the_item`, whose tasks file has a behavior group (`Kind: behavior.`) that says to add the guard `_delivery_title`, and assert the item is New with the definition line in the comment, that no step ran and that the worktree holds no change. Add `test_a_behavior_group_whose_guard_the_base_lacks_runs`, which asserts the group runs and commits. Run both first and see the first fail +- [x] 2.1 In `tests/roadmap/test_roadmap_implement_job.py`, add `test_a_behavior_group_that_adds_a_guard_the_base_already_runs_returns_the_item`, whose tasks file has a behavior group (a failing test, then a task that adds the guard `_delivery_title`), and assert the item is New with the definition line in the comment, that no model request ran and that the branch tip is as it was. Add `test_a_behavior_group_whose_guard_the_base_lacks_runs`, which asserts the group runs and commits, and `test_a_group_that_is_not_behavior_runs_though_the_base_defines_the_name`. Add the table test of `added_guard` to `tests/roadmap/test_premise.py`. Run them first and see the first fail - Verification: `uv run devops ci test tests/roadmap/test_roadmap_implement_job.py` fails on the base -- [ ] 2.2 Expose `group_kind(group)` in `src/devops_cli/roadmap/plan_read.py`, the inference that `_group` already uses. Add `guard_names(group)` to `src/devops_cli/roadmap/implement_run.py`, which reads a task of an add verb, a word such as guard or check, and one backticked identifier. Before a behavior group starts, call `defines` of `src/devops_cli/roadmap/premise.py` at `self._head()` for each name, and take `_return_premise` with a `premise` request whose evidence is the definition line when one is found. A group of another kind runs no check +- [x] 2.2 Add `added_guard(task_text)` and `false_premises(source, sha, group)` to `src/devops_cli/roadmap/premise.py`. `added_guard` reads a task of an add verb, then the word guard or check directly followed by one identifier in a code span; an identifier after a preposition names code that the task changes. `false_premises` calls `definition_lines` for each such name. In `src/devops_cli/roadmap/implement_run.py`, call `false_premises` before the step of a group for which `TaskGroup.is_behavior` holds, at the last commit that is no `wip:` commit, and return the item to New with the definition lines when there are any. A group of another kind runs no check - Verification: `uv run devops ci test tests/roadmap/test_roadmap_implement_job.py` passes ## 3. Record the new premise check on the implement page -Reads: `docs/roadmap/implement.md`, `docs/ROADMAP_WORKFLOW.md`, `CONTEXT.md`. Changes: `docs/roadmap/implement.md`. +Reads: `docs/roadmap/implement.md`, `docs/ROADMAP_WORKFLOW.md`, `CONTEXT.md`. Changes: `docs/roadmap/implement.md`, `docs/ROADMAP_WORKFLOW.md`. -- [ ] 3.1 Add a paragraph to the page `docs/roadmap/implement.md` that says the job checks the premise of a behavior group at the branch tip before the group starts, that a definition of a guard the task adds returns the item to New with the definition line, and that the job writes nothing in that case +- [x] 3.1 Add a paragraph to the page `docs/roadmap/implement.md` that says the job checks the premise of a behavior group at the branch tip before the group starts, that a definition of a guard the task adds returns the item to New with the definition line, and that the job writes nothing in that case - Verification: `uv run devops ci docs` passes ## Workflow follow-up diff --git a/openspec/specs/roadmap-implement-job/spec.md b/openspec/specs/roadmap-implement-job/spec.md index dbd768a7c..923124c02 100644 --- a/openspec/specs/roadmap-implement-job/spec.md +++ b/openspec/specs/roadmap-implement-job/spec.md @@ -1382,3 +1382,29 @@ The report of a Job with no result SHALL say that the Service starts a new Job w #### Scenario: A Job that failed - **WHEN** the round reports a Job that ended Failed and left its item In Progress - **THEN** the comment names the setting `roadmap.implement.continuations_per_item` and the comment `implement: retry` + +### Requirement: The job checks the premise of a behavior group at the branch tip +Before the job runs a behavior group, the job SHALL check each guard or check identifier that a task of the group adds against the branch tip, and SHALL return the item to New with the definition line when the branch tip defines it. The job SHALL read the kind of the group from `TaskGroup.is_behavior` of `src/devops_cli/roadmap/step_files.py`. + +#### Scenario: A task adds a guard that the base already runs +- **WHEN** A behavior group task says to add the guard `_delivery_title` and the branch tip defines `_delivery_title` +- **THEN** The job runs no group step, writes no group file, makes no group commit, and comments the definition line with the return to New. + +#### Scenario: A task adds a guard that the base lacks +- **WHEN** A behavior group task says to add the guard `_missing_guard` and the branch tip defines no `_missing_guard` +- **THEN** The job runs the group and commits the file that the group writes. + +#### Scenario: A group that is not behavior adds a defined name +- **WHEN** A group whose kind is not behavior has a task that says to add the name `_delivery_title`, which the branch tip defines +- **THEN** The job returns no item to New and runs the group. + +### Requirement: One shared definition lookup reports a definition at a commit +The stage job and the implement job SHALL use the definition lookup of `src/devops_cli/roadmap/premise.py`: `definition_lines` returns the lines that define an identifier at a commit, and `defines` reports whether that list is not empty. + +#### Scenario: Both jobs get the definition lines from one function +- **WHEN** The stage job probes a cited identifier and the implement job checks a task at the same commit +- **THEN** Both jobs get the same definition lines from `definition_lines` of `src/devops_cli/roadmap/premise.py`. + +#### Scenario: The check writes nothing +- **WHEN** The implement job checks the tasks of a behavior group at the branch tip +- **THEN** The check runs only read-only git commands and the worktree holds no change. diff --git a/src/devops_cli/lang/en/messages.py b/src/devops_cli/lang/en/messages.py index c053f2e14..8fa4ab3c5 100644 --- a/src/devops_cli/lang/en/messages.py +++ b/src/devops_cli/lang/en/messages.py @@ -2957,6 +2957,14 @@ class RoadmapMessages: "of the issue does not hold, so the job wrote no compensating code.\n\n" "Evidence:\n\n{evidence}" ) + implement_comment_premise_defined: str = ( + "{marker}\nThe implement job returned the item to New. A task of a behavior group adds " + "a guard that the branch already defines, so the job ran no step and wrote no code.\n\n" + "Evidence:\n\n{evidence}" + ) + premise_guard_defined: str = ( + '- The task "{task}" adds `{name}`. The branch already defines it: {line}' + ) implement_stop_question_empty_premise: str = ( "The agent asked to return the item on a premise failure, but gave no evidence." ) diff --git a/src/devops_cli/roadmap/implement_run.py b/src/devops_cli/roadmap/implement_run.py index 070e17add..dbce5e057 100644 --- a/src/devops_cli/roadmap/implement_run.py +++ b/src/devops_cli/roadmap/implement_run.py @@ -148,8 +148,9 @@ class of the failure, the action taken and the action left (`failure.py`, #3065) last_failing_command, summary_table, ) -from devops_cli.roadmap.implement_wip import WipCommit, kept_commits +from devops_cli.roadmap.implement_wip import WipCommit, kept_commits, wip_base from devops_cli.roadmap.plan_read import PlanReadError, merged_recorded_tests +from devops_cli.roadmap.premise import false_premises from devops_cli.roadmap.pull_form import title_problems from devops_cli.roadmap.pull_labels import ( add_pull_labels, @@ -673,12 +674,26 @@ def _require_unclaimed(self, boundary: str) -> None: raise _ClaimedError(owner, boundary) def _return_premise(self, request: PlanRequest) -> None: - texts = MESSAGES.roadmap - evidence = mask_secrets(request.evidence.strip())[:CONST_ROADMAP_IMPLEMENT_REASON_CHARS] - comment = texts.implement_comment_premise_failed.format( - marker=CONST_ROADMAP_IMPLEMENT_COMMENT_MARKER, - evidence=evidence, - ) + self._return_to_new(MESSAGES.roadmap.implement_comment_premise_failed, request.evidence) + + def _check_premise(self, group: TaskGroup) -> bool: + """Return the item to New, and say so, when a task of the behavior group `group` adds a guard + that the branch already defines (#3417). The check reads the last commit that is no `wip:` + commit, so the guard that an earlier attempt of this group kept is no defect of the plan. It + runs before the step, so a false premise costs no model request and writes no file. A group + of another kind adds no product guard, so it gets no check.""" + if not group.is_behavior: + return False + evidence = false_premises(self.path, wip_base(self._git), group) + if not evidence: + return False + self._return_to_new(MESSAGES.roadmap.implement_comment_premise_defined, "\n".join(evidence)) + return True + + def _return_to_new(self, template: str, evidence: str) -> None: + """Return the item to New with a comment of `template`, and clear the record of the run.""" + evidence = mask_secrets(evidence.strip())[:CONST_ROADMAP_IMPLEMENT_REASON_CHARS] + comment = template.format(marker=CONST_ROADMAP_IMPLEMENT_COMMENT_MARKER, evidence=evidence) apply_return(self.store, plan_return(self.store, self.number, comment, self.repo)) self.store.set_marks( self._fresh(), @@ -907,6 +922,8 @@ def _groups(self) -> None: self._require_unclaimed( MESSAGES.roadmap.implement_boundary_group.format(group=group.number) ) + if self._check_premise(group): + return try: outcome = run_step( self.store, diff --git a/src/devops_cli/roadmap/premise.py b/src/devops_cli/roadmap/premise.py new file mode 100644 index 000000000..6c8a7f7b0 --- /dev/null +++ b/src/devops_cli/roadmap/premise.py @@ -0,0 +1,104 @@ +"""The definition check that the stage probe and the implement job share (#3417). + +A name is defined at a commit when a Python file of that commit holds its `def`, `async def` or +`class`, or an assignment of it at the start of a line. The stage probe asks the staging commit +whether an identifier that the issue cites is defined (#3011). The implement job asks the branch +tip whether a guard that a behavior group adds is already there (#3417). Both ask one question, so +both call this module and run one `git grep` at the commit. + +The check reads the commit, never the working tree: a line that only an uncommitted change holds +is no definition of the commit. +""" + +from __future__ import annotations + +import keyword +import re +from collections.abc import Callable +from typing import TYPE_CHECKING + +from devops_cli.lang import MESSAGES +from devops_cli.roadmap.refine import SYMBOL_PATHSPEC, git_result +from devops_cli.roadmap.step_files import TaskGroup + +if TYPE_CHECKING: + from pathlib import Path + +# One git command in a checkout: the arguments after `git -C `, and its exit code with its +# standard output. +GitResult = Callable[..., tuple[int, str]] + + +def _definition_pattern(name: str) -> str: + """The extended expression for a definition of `name`: a `def`, an `async def` or a `class`, or + an assignment at the start of a line, with or without an annotation. `name` is an identifier, + so it holds no character that the expression reads.""" + return ( + rf"(^|[^A-Za-z0-9_])(def|class)[[:space:]]+{name}([^A-Za-z0-9_]|$)" + rf"|^[[:space:]]*{name}[[:space:]]*(:[^=]*)?=" + ) + + +def definition_lines( + source: Path, sha: str, name: str, git: GitResult = git_result +) -> tuple[str, ...]: + """The lines of the definitions of `name` at commit `sha` of the checkout `source`, as + `:::`. Empty when the commit defines no such name. `git` runs one + command of a caller that keeps a budget of them.""" + code, out = git( + source, "grep", "-n", "-E", "-e", _definition_pattern(name), sha, "--", SYMBOL_PATHSPEC + ) + return tuple(out.splitlines()) if code == 0 and out else () + + +def defines(source: Path, sha: str, name: str, git: GitResult = git_result) -> bool: + """Whether the commit `sha` of the checkout `source` defines `name`.""" + return bool(definition_lines(source, sha, name, git)) + + +# A task names the guard it adds when the code span directly follows the word guard or check, as in +# "Add the guard `name`" or "Create a check named `name`". A span after a preposition ("Add a check +# to `run_step`") names code that the task changes, not code that it adds. +_ADD_VERB = re.compile(r"\b(?:add|adds|create|creates|introduce|introduces)\b", re.IGNORECASE) +_GUARD_SPAN = re.compile( + r"\b(?:guards?|checks?)\s+(?:(?:named|called)\s+)?`([^`]*)`", re.IGNORECASE +) + + +def added_guard(task_text: str) -> str | None: + """The name of the guard that the task adds, or None. A task adds a guard when an add verb is + followed by the word guard or check and then by one code span that holds an identifier (a path, + a command or a phrase is no identifier). A span after any other word names code that the task + changes. A task with two such names does not say which one it adds, so it names none.""" + verb = _ADD_VERB.search(task_text) + if verb is None: + return None + names = { + span + for span in _GUARD_SPAN.findall(task_text, verb.end()) + if span.isidentifier() and not keyword.iskeyword(span) + } + return names.pop() if len(names) == 1 else None + + +def false_premises( + source: Path, sha: str, group: TaskGroup, git: GitResult = git_result +) -> tuple[str, ...]: + """The evidence for each task of `group` that adds a guard which commit `sha` already defines: + the task, the name and the definition line. Empty when the premise of the group holds. The + check reads the commit and writes nothing.""" + found: list[str] = [] + for task in group.tasks: + name = added_guard(task.text) + if name is None: + continue + if lines := definition_lines(source, sha, name, git): + found.append( + MESSAGES.roadmap.premise_guard_defined.format( + task=task.text.strip(), name=name, line=lines[0] + ) + ) + return tuple(found) + + +__all__ = ["added_guard", "defines", "definition_lines", "false_premises"] diff --git a/src/devops_cli/roadmap/stage_probe.py b/src/devops_cli/roadmap/stage_probe.py index a5b1141c4..e5e295009 100644 --- a/src/devops_cli/roadmap/stage_probe.py +++ b/src/devops_cli/roadmap/stage_probe.py @@ -30,6 +30,7 @@ CONST_ROADMAP_STAGE_PROBE_MAX_COMMANDS, ) from devops_cli.lang import MESSAGES +from devops_cli.roadmap.premise import definition_lines from devops_cli.roadmap.refine import ( SYMBOL_PATHSPEC, cited_identifiers, @@ -45,16 +46,6 @@ _LOG_FORMAT: Final[str] = "--format=%h %as %s" -def _definition_pattern(name: str) -> str: - """The extended expression for a definition of `name`: a `def`, an `async def` or a `class`, or - an assignment at the start of a line, with or without an annotation. `name` is an identifier, - so it holds no character that the expression reads.""" - return ( - rf"(^|[^A-Za-z0-9_])(def|class)[[:space:]]+{name}([^A-Za-z0-9_]|$)" - rf"|^[[:space:]]*{name}[[:space:]]*(:[^=]*)?=" - ) - - @dataclass(frozen=True) class Probe: """What the probe found: the lines for the model, the cited items that are missing, the items @@ -91,10 +82,10 @@ class _Run: skipped: list[str] = field(default_factory=list) _tree: StagedTree | None = None - def git(self, *args: str) -> tuple[int, str]: - """Run one command of the budget.""" + def git(self, source: Path, *args: str) -> tuple[int, str]: + """Run one command of the budget in `source`, which is the checkout of the run.""" self.left -= 1 - return git_result(self.source, *args) + return git_result(source, *args) def same_name(self, name: str) -> list[str]: """The files of the staging commit with the name of `name`, sorted and at most @@ -103,7 +94,7 @@ def same_name(self, name: str) -> list[str]: if self._tree is None: if self.left < 1: return [] - code, out = self.git("ls-tree", "-r", "-z", "--name-only", self.sha) + code, out = self.git(self.source, "ls-tree", "-r", "-z", "--name-only", self.sha) self._tree = ( StagedTree(path for path in out.split("\0") if path) if code == 0 @@ -114,12 +105,12 @@ def same_name(self, name: str) -> list[str]: def newest_commit(self, *args: str) -> str: """The newest commit that `git log` finds with `args` at the staging commit, as `hash date subject`, or an empty text when it finds none (the command costs one of the budget).""" - code, out = self.git("log", "-n1", _LOG_FORMAT, *args) + code, out = self.git(self.source, "log", "-n1", _LOG_FORMAT, *args) return out.strip() if code == 0 else "" def path(self, name: str) -> None: texts = MESSAGES.roadmap - code, _ = self.git("cat-file", "-e", f"{self.sha}:{name}") + code, _ = self.git(self.source, "cat-file", "-e", f"{self.sha}:{name}") if code == 0: self.lines.append(texts.stage_probe_path_present.format(path=name)) return @@ -152,11 +143,10 @@ def _removal(self, name: str) -> str: def identifier(self, name: str) -> None: texts = MESSAGES.roadmap - code, out = self.git( - "grep", "-n", "-E", "-e", _definition_pattern(name), self.sha, "--", SYMBOL_PATHSPEC - ) - if code == 0 and out: - hits = out.splitlines()[:CONST_ROADMAP_STAGE_PROBE_DEFINITION_LINES] + hits = definition_lines(self.source, self.sha, name, self.git)[ + :CONST_ROADMAP_STAGE_PROBE_DEFINITION_LINES + ] + if hits: for hit in hits: where = hit.removeprefix(f"{self.sha}:").strip() self.lines.append(texts.stage_probe_defined.format(name=name, where=where)) diff --git a/tests/roadmap/test_premise.py b/tests/roadmap/test_premise.py new file mode 100644 index 000000000..f340b8190 --- /dev/null +++ b/tests/roadmap/test_premise.py @@ -0,0 +1,44 @@ +"""The definition check that the stage probe and the implement job share (#3417).""" + +from __future__ import annotations + +import pytest + +from devops_cli.roadmap.premise import added_guard +from tests.support.paths import REPO_ROOT + + +def test_the_definition_lines_of_an_identifier_come_from_the_commit() -> None: + from devops_cli.roadmap.premise import definition_lines + + lines = definition_lines(REPO_ROOT, "HEAD", "_delivery_title") + + assert any( + line.startswith("HEAD:src/devops_cli/roadmap/implement_run.py:") + and "_delivery_title" in line + for line in lines + ) + + +@pytest.mark.parametrize( + ("task", "name"), + [ + ( + "- [ ] 1.2 Add the guard `_delivery_title` to `src/devops_cli/shout.py`", + "_delivery_title", + ), + ("- [ ] 1.2 Create a check `title_ok` in `src/devops_cli/shout.py`", "title_ok"), + ("- [ ] 1.2 Add `shout` to `src/devops_cli/shout.py`", None), + ("- [ ] 1.2 Change the guard `_delivery_title` in `src/devops_cli/shout.py`", None), + ("- [ ] 1.2 Add the guard `one` and the check `two`", None), + ("- [ ] 1.1 Add the failing test `test_title_check` for `src/devops_cli/shout.py`", None), + ("- [ ] 1.2 Add a guard to `run_step` that refuses an empty plan", None), + ("- [ ] 1.2 Add a check in `_return_premise` for a blank title", None), + ("- [ ] 1.2 Add a check to `apply_return` that reads the item", None), + ("- [ ] 1.2 Create a check named `title_ok` in `src/devops_cli/shout.py`", "title_ok"), + ], +) +def test_a_task_adds_a_guard_only_when_the_identifier_follows_the_guard_word( + task: str, name: str | None +) -> None: + assert added_guard(task) == name diff --git a/tests/roadmap/test_roadmap_implement_job.py b/tests/roadmap/test_roadmap_implement_job.py index 8ea511961..b448ba948 100644 --- a/tests/roadmap/test_roadmap_implement_job.py +++ b/tests/roadmap/test_roadmap_implement_job.py @@ -1363,6 +1363,140 @@ def test_the_pr_comment_shows_a_fail_to_pass_line_for_each_behavior_group( ) in comment["body"] +# ── The premise of a behavior group (#3417) ─────────────────────────────────────────────── + +TITLES = '"""The titles."""\n\n\ndef _delivery_title(text: str) -> str:\n return text.strip()\n' +TITLES_PATH = "src/devops_cli/titles.py" +TITLES_LINE = f"{TITLES_PATH}:4:def _delivery_title(text: str) -> str:" + + +def _guard_tasks(name: str, *, behavior: bool) -> str: + """A change of two groups whose first group adds the guard `name`. With `behavior`, the first + group is a behavior group: a failing test, then the guard. Otherwise it is a group of kind + `other` with one task that adds the guard.""" + if behavior: + first = ( + "Reads: `CONTEXT.md`. Changes: `tests/test_shout.py` (new), " + "`src/devops_cli/shout.py` (new).\n\n" + "- [ ] 1.1 Add the failing test `tests/test_shout.py` for `src/devops_cli/shout.py`\n" + " - Verification: `uv run devops ci test tests/test_shout.py` fails on the base\n" + f"- [ ] 1.2 Add the guard `{name}` to `src/devops_cli/shout.py`\n" + " - Verification: `uv run devops ci test tests/test_shout.py`\n" + ) + else: + first = ( + "Reads: `CONTEXT.md`. Changes: `src/devops_cli/shout.py` (new), `tests/test_shout.py` (new).\n\n" + f"- [ ] 1.1 Add the guard `{name}` to `src/devops_cli/shout.py`\n" + " - Verification: `uv run devops ci test tests/test_shout.py`\n" + ) + return ( + f"# Tasks\n\n## 1. The first group\n\n{first}\n" + "## 2. The second group\n\n" + "Reads: `CONTEXT.md`. Changes: `CONTEXT.md`.\n\n" + "- [ ] 2.1 Document the function in `CONTEXT.md`\n" + " - Verification: `uv run devops docs check`\n\n" + "## Criteria\n\n- Group 1: The job keeps every comment.\n" + ) + + +def _ticked(tasks: str) -> tuple[str, str]: + """The tasks with group 1 ticked, and with both groups ticked.""" + first = tasks.replace("- [ ] 1.", "- [x] 1.") + return first, first.replace("- [ ] 2.", "- [x] 2.") + + +class _PremiseWorld: + """A staged branch whose base defines `_delivery_title` in `src/devops_cli/titles.py`.""" + + @pytest.fixture + def staged_files(self) -> dict[str, str]: + return {"tests/conftest.py": '"""The tests."""\n', TITLES_PATH: TITLES} + + +class TestAFalsePremiseOfABehaviorGroup(_PremiseWorld): + """A task of a behavior group adds a guard that the branch tip defines (#3417).""" + + @pytest.fixture + def staged_tasks(self) -> str: + return _guard_tasks("_delivery_title", behavior=True) + + def test_a_behavior_group_that_adds_a_guard_the_base_already_runs_returns_the_item( + self, job_world: JobWorld + ) -> None: + tip = job_world.repo.origin_git("rev-parse", BRANCH) + script = two_groups() + + applied = job_world.run(script) + + assert (applied.ready, applied.stopped, applied.failed) == ((), (), ()) + status, record = _item(job_world) + assert (status, record.get("implement.group"), record.get("implement.branch")) == ( + "New", + None, + None, + ) + (comment,) = job_world.issue_comments() + assert TITLES_LINE in comment + assert "`_delivery_title`" in comment + assert script.seen == [] + assert job_world.repo.origin_git("rev-parse", BRANCH) == tip + + +class TestATruePremiseOfABehaviorGroup(_PremiseWorld): + """A task of a behavior group adds a guard that the branch tip lacks (#3417).""" + + @pytest.fixture + def staged_tasks(self) -> str: + return _guard_tasks("_missing_guard", behavior=True) + + def test_a_behavior_group_whose_guard_the_base_lacks_runs(self, job_world: JobWorld) -> None: + answer_pytest_on_base(job_world.programs, junit((SHOUT_TEST_ID, "failed"))) + first, both = _ticked(_guard_tasks("_missing_guard", behavior=True)) + script = ModelScript( + tool_calls( + write_call("tests/test_shout.py", TEST_BODY), + write_call("src/devops_cli/shout.py", SHOUT), + write_call(TASKS_PATH, first), + ), + say("Group 1 is done."), + tool_calls(*group_two_calls(both)), + say("Group 2 is done."), + ) + + applied = job_world.run(script) + + assert (applied.ready, applied.stopped, applied.failed) == ((1,), (), ()) + assert "src/devops_cli/shout.py" in job_world.repo.origin_git( + "diff", "--name-only", f"main..{BRANCH}" + ) + + +class TestAGuardOfAGroupThatIsNotBehavior(_PremiseWorld): + """A group of kind `other` names a defined guard: the check does not apply to it (#3417).""" + + @pytest.fixture + def staged_tasks(self) -> str: + return _guard_tasks("_delivery_title", behavior=False) + + def test_a_group_that_is_not_behavior_runs_though_the_base_defines_the_name( + self, job_world: JobWorld + ) -> None: + _, both = _ticked(_guard_tasks("_delivery_title", behavior=False)) + script = ModelScript( + tool_calls( + write_call("src/devops_cli/shout.py", SHOUT), + write_call(TASKS_PATH, both.replace("- [x] 2.", "- [ ] 2.")), + ), + say("Group 1 is done."), + tool_calls(*group_two_calls(both)), + say("Group 2 is done."), + ) + + applied = job_world.run(script) + + assert (applied.ready, applied.stopped, applied.failed) == ((1,), (), ()) + + class TestVerifyOutputOfAnOtherGroup: """The job of a group of kind `other`: its verify command prints a line (#3042)."""