diff --git a/scripts/e2e/README.md b/scripts/e2e/README.md index bb732de838..8883f7d720 100644 --- a/scripts/e2e/README.md +++ b/scripts/e2e/README.md @@ -202,6 +202,15 @@ and are often `0600`, so a second container run copies `out/` and `data/logs/` i fails if any secret survives. Only `upload/` is uploaded, and only when redaction succeeded. +When a test does not pass, the harness preserves plans from each pipeline clone under +`out/pipeline-artifacts///` and Claude Code JSONL under +`out/transcripts/`, retaining relative paths. The transcripts include the orchestrator, +skill sessions and their subagents; the orchestrator's transcript holds each raw +`bind_plan_set` response and its `plan_parts` argument. These files go through the same +redaction and upload steps as the other output artifacts. Collection also runs for an +expected failure or a harness exception. A container killed before or during collection +may leave no evidence or only partial evidence. + `clean-install` mounts no home directories and retains evidence under `out/`. Its redactor receives the same required `--secret-env` names with variables unset and skips the absent home-log tree. Redaction and upload still run after test failures. @@ -330,6 +339,25 @@ docker run --rm \ python3 /opt/e2e/e2e_harness.py run --test clean-install --out /artifacts ``` +### Check a real plan against the plan-set gate before dispatching a pipeline scenario + +After the plan-set changes have merged, perform this check before CI commissioning: + +1. Check out the #5255 branch revision containing both the `implementation` E2E scenario + and the evidence collector. Build and run that revision with the Docker + invocation above, selecting `--test implementation`. Use the same revision for + commissioning and record the model. +2. If the run reaches implementation, the first bind succeeded. If the test does not + pass, inspect the orchestrator's `bind_plan_set` results in `out/transcripts/`. + An observed `success: true` on the first or second bind is the pass condition. + A `coverage_failed` result explains the rejection in `coverage_gaps`, and + `out/pipeline-artifacts/` retains the rejected plan. Missing evidence is inconclusive + and does not satisfy this gate: require an observed successful bind. When a run + replans, also confirm the second bind's `plan_parts` names only the replacement plan. +3. Record the model, plan path and bind result in issue #5259's body using + `gh issue edit --body-file`, never as a comment. Only an observed successful bind + unblocks `gh workflow run e2e.yml -f tests=implementation`. + ### Sandbox smoke with teardown Set `MINIMAX_API_KEY` and `E2E_SANDBOX_TOKEN` in the local environment first. Build the same diff --git a/scripts/e2e/e2e_harness.py b/scripts/e2e/e2e_harness.py index 3a89b895de..157f948203 100644 --- a/scripts/e2e/e2e_harness.py +++ b/scripts/e2e/e2e_harness.py @@ -5,6 +5,7 @@ import argparse import ast +import fnmatch import json import os import re @@ -35,6 +36,7 @@ REDACTED = b"[REDACTED]" CONFIG_TEMPLATE = Path(__file__).with_name("autoskillit-config.yaml") SANDBOX_CLONE = Path("/workspace/sandbox") +PIPELINE_RUNS_DIR = "autoskillit-runs" _SCRUBBED_ENV = frozenset({"CLAUDE_CODE_OAUTH_TOKEN", *SECRET_ENV}) _STDERR_TAIL_CHARS = 2000 @@ -1017,6 +1019,67 @@ def _run_test( return run_recipe(test, catalog.sandbox_repository, token, out, scrubbed, runner) +def _copy_failure_files( + source: Path, dest: Path, boundary: Path, pattern: str, errors: list[str] +) -> int: + relative = source.relative_to(boundary) + try: + if any( + boundary.joinpath(*relative.parts[:depth]).is_symlink() + for depth in range(len(relative.parts) + 1) + ): + return 0 + if not source.exists(): + return 0 + except OSError as exc: + errors.append(f"{source}: {exc}") + return 0 + copied = 0 + for root, dirs, files in os.walk( + source, followlinks=False, onerror=lambda exc: errors.append(f"{source}: {exc}") + ): + dirs.sort() + for name in sorted(fnmatch.filter(files, pattern)): + path = Path(root) / name + try: + if path.is_symlink() or not path.is_file(): + continue + target = dest / path.relative_to(source) + target.parent.mkdir(parents=True, exist_ok=True) + shutil.copyfile(path, target) + copied += 1 + except OSError as exc: + errors.append(f"{path}: {exc}") + return copied + + +def collect_failure_evidence(out: Path, home: Path) -> dict[str, Any]: + """Keep pipeline plans and Claude transcripts for an unsuccessful E2E run.""" + errors: list[str] = [] + runs = SANDBOX_CLONE.parent / PIPELINE_RUNS_DIR + try: + clones = [] if runs.is_symlink() else sorted(runs.iterdir()) + except FileNotFoundError: + clones = [] + except OSError as exc: + errors.append(f"{runs}: {exc}") + clones = [] + plan_files = 0 + for clone in clones: + for kind in ("make-plan", "rectify"): + plan_files += _copy_failure_files( + clone / ".autoskillit" / "temp" / kind, + out / "pipeline-artifacts" / clone.name / kind, + clone, + "*", + errors, + ) + transcripts = _copy_failure_files( + home / ".claude" / "projects", out / "transcripts", home, "*.jsonl", errors + ) + return {"plan_files": plan_files, "transcripts": transcripts, "errors": errors} + + def run_test( test: e2e_catalog.CatalogTest, catalog: e2e_catalog.Catalog, @@ -1043,6 +1106,18 @@ def run_test( result.update( outcome=outcome, passed=outcome == "passed", expected_findings=matched_findings ) + if not result["passed"]: + try: + evidence = collect_failure_evidence(out, home) + except Exception as exc: + evidence = { + "plan_files": 0, + "transcripts": 0, + "errors": [f"{type(exc).__name__}: {exc}"], + "exception": traceback.format_exc(), + } + if any(evidence.values()): + result["evidence"] = evidence (out / "result.json").write_text(json.dumps(result, indent=2) + "\n", encoding="utf-8") return failures diff --git a/src/autoskillit/core/io/markdown_sections.py b/src/autoskillit/core/io/markdown_sections.py index 003039e709..80225fe996 100644 --- a/src/autoskillit/core/io/markdown_sections.py +++ b/src/autoskillit/core/io/markdown_sections.py @@ -18,7 +18,9 @@ def extract_section(markdown: str, heading: str) -> str: pattern = re.compile(rf"^## {re.escape(heading)}[ \t]*$", re.MULTILINE) matches = tuple(pattern.finditer(markdown)) if len(matches) != 1: - raise ValueError(f"expected exactly one ## {heading} section") + raise ValueError( + f"expected exactly one ## {heading} section; found {len(matches)} matching headings" + ) start = matches[0].end() next_heading = re.search(r"^##\s+", markdown[start:], re.MULTILINE) end = start + next_heading.start() if next_heading is not None else len(markdown) @@ -29,7 +31,7 @@ def split_table_row(line: str) -> tuple[str, ...]: """Parse one strictly pipe-delimited row.""" stripped = line.strip() if not stripped.startswith("|") or not stripped.endswith("|"): - raise ValueError("table rows must be pipe-delimited") + raise ValueError(f'table rows must be pipe-delimited; offending line: "{stripped[:120]}"') return tuple(cell.strip() for cell in stripped[1:-1].split("|")) @@ -41,15 +43,24 @@ def parse_pipe_table( lines = tuple(line for line in section.splitlines() if line.strip()) header = tuple(expected_header) if len(lines) < 2: - raise ValueError("table must contain a header and separator") + line = lines[0].strip()[:120] if lines else "" + raise ValueError(f'table must contain a header and separator; offending line: "{line}"') if split_table_row(lines[0]) != header: - raise ValueError(f"table header must be {header!r}") + raise ValueError( + f'table header must be {header!r}; offending line: "{lines[0].strip()[:120]}"' + ) separator = split_table_row(lines[1]) if len(separator) != len(header) or any( re.fullmatch(r":?-{3,}:?", cell) is None for cell in separator ): - raise ValueError("table separator is invalid") - rows = tuple(split_table_row(line) for line in lines[2:]) - if any(len(row) != len(header) for row in rows): - raise ValueError(f"table rows must have exactly {len(header)} columns") - return rows + raise ValueError(f'table separator is invalid; offending line: "{lines[1].strip()[:120]}"') + rows = [] + for line in lines[2:]: + row = split_table_row(line) + if len(row) != len(header): + raise ValueError( + f"table rows must have exactly {len(header)} columns; " + f'offending line: "{line.strip()[:120]}"' + ) + rows.append(row) + return tuple(rows) diff --git a/src/autoskillit/core/planset/allocation.py b/src/autoskillit/core/planset/allocation.py index af085f3e47..2becede91f 100644 --- a/src/autoskillit/core/planset/allocation.py +++ b/src/autoskillit/core/planset/allocation.py @@ -21,16 +21,29 @@ def parse_part_allocation(part_markdown: str, *, part_key: str) -> tuple[Allocat rows = parse_pipe_table(section, _HEADER) result: list[AllocationRowDef] = [] seen: set[str] = set() - for requirement_id, allocation, implementation_step in rows: + raw_rows = tuple(line for line in section.splitlines() if line.strip())[2:] + for (requirement_id, allocation, implementation_step), raw_row in zip( + rows, raw_rows, strict=True + ): + if not requirement_id: + raise ValueError( + f'Requirement ID is empty in row "{raw_row.strip()[:120]}"; ' + "use a non-empty bare Requirement ID" + ) if requirement_id in seen: raise ValueError(f"duplicate allocated requirement {requirement_id}") seen.add(requirement_id) if not implementation_step: - raise ValueError("allocation implementation step is empty") + raise ValueError( + f"{requirement_id}: allocation implementation step is empty; " + "use one matching heading key such as Step 2.1" + ) try: kind = AllocationKind(allocation.lower()) except ValueError as exc: - raise ValueError(f"unknown allocation {allocation!r}") from exc + raise ValueError( + f'{requirement_id}: unknown allocation "{allocation}"; use owned or shared' + ) from exc result.append( AllocationRowDef( requirement_id=requirement_id, @@ -45,7 +58,7 @@ def parse_part_allocation(part_markdown: str, *, part_key: str) -> tuple[Allocat def verify_allocation_evidence( part_markdown: str, rows: tuple[AllocationRowDef, ...], -) -> tuple[tuple[str, PlanSetRejectReason], ...]: +) -> tuple[tuple[str, PlanSetRejectReason, str], ...]: """Ensure an allocation names a real step whose own text names the requirement.""" matches = tuple(STEP_HEADING_RE.finditer(part_markdown)) blocks: dict[str, str] = {} @@ -57,11 +70,30 @@ def verify_allocation_evidence( ) end = min(next_step, section_end) blocks[match.group(1)] = part_markdown[match.start() : end] - errors: list[tuple[str, PlanSetRejectReason]] = [] + errors: list[tuple[str, PlanSetRejectReason, str]] = [] for row in rows: block = blocks.get(row.implementation_step) if block is None: - errors.append((row.requirement_id, PlanSetRejectReason.ALLOCATION_STEP_MISSING)) + found = ( + f"step keys found: {', '.join(blocks)}" if blocks else "no step heading was found" + ) + errors.append( + ( + row.requirement_id, + PlanSetRejectReason.ALLOCATION_STEP_MISSING, + f'{row.requirement_id}: Implementation Step cell "{row.implementation_step}" ' + f"does not match a step; {found}. Expected exactly one key such as Step 2.1, " + "copied from a ### Step N: title heading (### Step N.M: title for sub-steps).", + ) + ) elif row.requirement_id not in block: - errors.append((row.requirement_id, PlanSetRejectReason.ALLOCATION_STEP_UNREFERENCED)) + errors.append( + ( + row.requirement_id, + PlanSetRejectReason.ALLOCATION_STEP_UNREFERENCED, + f'Requirement ID cell "{row.requirement_id}" is not referenced in ' + f"{row.implementation_step}; the bare ID must appear in that step's own " + "text, on the line under its heading.", + ) + ) return tuple(errors) diff --git a/src/autoskillit/core/types/audit/_type_plan_set_authority.py b/src/autoskillit/core/types/audit/_type_plan_set_authority.py index 2a0ef475e4..cdcaef7e1b 100644 --- a/src/autoskillit/core/types/audit/_type_plan_set_authority.py +++ b/src/autoskillit/core/types/audit/_type_plan_set_authority.py @@ -440,12 +440,21 @@ def _validate_collections(self) -> None: object.__setattr__(self, "unparsed_marker_lines", tuple(self.unparsed_marker_lines)) if not isinstance(self.coverage, CoverageResultDef): raise ValueError("PlanSetAuthority.coverage is invalid") + part_keys = {part.part_key for part in self.parts} + if not set(self.coverage.parts_without_obligations) <= part_keys: + raise ValueError("coverage.parts_without_obligations contains an unknown part key") if self.state is PlanSetState.SEALED and self.coverage.status is not CoverageStatus.PASS: raise ValueError("sealed authority requires passing coverage") if tuple(part.ordinal for part in self.parts) != tuple(range(1, len(self.parts) + 1)): raise ValueError("PlanSetAuthority.parts must have consecutive ordinals") if len({Path(part.locator).resolve() for part in self.parts}) != len(self.parts): raise ValueError("PlanSetAuthority.parts contain duplicate locators") + if self.inventory_mode in {InventoryMode.UNENUMERATED, InventoryMode.NO_ISSUE}: + empty_parts = part_keys - {row.part_key for row in self.allocations} + if set(self.coverage.parts_without_obligations) != empty_parts: + raise ValueError( + "coverage.parts_without_obligations must match actual empty parts" + ) @classmethod def create( @@ -622,6 +631,14 @@ class PlanSetBindResult: first_part_path: str = "" coverage_gaps: str = "" + def __post_init__(self) -> None: + if self.success != (self.reason is None): + raise ValueError("successful results require no reason; failures require a reason") + if self.reason is not PlanSetRejectReason.COVERAGE_FAILED and self.coverage_gaps: + raise ValueError("only coverage_failed results may contain coverage_gaps") + if self.reason is PlanSetRejectReason.COVERAGE_FAILED and not self.coverage_gaps.strip(): + raise ValueError("coverage_failed requires non-empty coverage_gaps") + def to_dict(self) -> dict[str, Any]: result = {name: getattr(self, name) for name in self.__dataclass_fields__} result["reason"] = self.reason.value if self.reason else "" diff --git a/src/autoskillit/recipe/api_orchestration/_api_orchestration_match.py b/src/autoskillit/recipe/api_orchestration/_api_orchestration_match.py index 90153a047e..a2cf3b1ee1 100644 --- a/src/autoskillit/recipe/api_orchestration/_api_orchestration_match.py +++ b/src/autoskillit/recipe/api_orchestration/_api_orchestration_match.py @@ -17,6 +17,7 @@ _LoadPipelineInputs, _ValidationResult, ) +from autoskillit.recipe.helpers._io_loading import _substitute_recipe_notes_in_yaml from autoskillit.recipe.io import ( RecipeInfo, find_recipe_by_name, @@ -45,6 +46,7 @@ def _resolve_recipe_match( raw_declared = match.content if match.content is not None else match.path.read_text() raw = substitute_temp_placeholder_yaml_safe(raw_declared, pipeline_inputs.temp_dir_relpath) raw = substitute_scripts_placeholder_yaml_safe(raw) + raw = _substitute_recipe_notes_in_yaml(raw) if match.source == RecipeSource.BUILTIN: recipes_dir = _orch.pkg_root() / "recipes" diff --git a/src/autoskillit/recipe/helpers/_io_loading.py b/src/autoskillit/recipe/helpers/_io_loading.py index ffd8f6190b..d799ad8099 100644 --- a/src/autoskillit/recipe/helpers/_io_loading.py +++ b/src/autoskillit/recipe/helpers/_io_loading.py @@ -13,8 +13,10 @@ LoadReport, LoadResult, RecipeSource, + compose_yaml, fast_loads, get_logger, + is_yaml_mapping_node, load_yaml, pkg_root, ) @@ -25,7 +27,7 @@ # Canonical recipe step note prose, single-sourced here so a doctrinal update -# lands in one place instead of eight recipe YAML/JSON copies. +# lands in one place instead of repeated recipe YAML/JSON copies. ANALYZE_PIPELINE_HEALTH_ERROR_NOTE: str = ( "Preserve the originating failure response separately. Inspect the diagnostic " "response and include its non-empty result verbatim as supplementary evidence. " @@ -39,16 +41,57 @@ _RECIPE_NOTE_TABLE: dict[str, str] = { _RECIPE_NOTE_ANALYZE_PIPELINE_HEALTH_ERROR: ANALYZE_PIPELINE_HEALTH_ERROR_NOTE, + "__RECIPE_NOTE_PLAN_SET_RENEWAL_FAILURE__": ( + "coverage_failed here is terminal by decision. The part passed the plan-set gate " + "before the walkthrough edited it, and a walkthrough cannot be re-run until its " + "edits are renewed. Relay the result's coverage_gaps in the failure diagnostic: " + "it names the rows the edit broke." + ), + "__RECIPE_NOTE_PLAN_PARTS_BINDING__": ( + "Bind exactly the plan_parts value captured from the producer that just ran. " + "A successful replan replaces the earlier list with its returned list: do not " + "append rejected paths or rebuild the list from a directory listing. When " + "salvage_plan is the producer, bind its returned list unchanged under the " + "existing salvage recovery and strict coverage checks." + ), } def _substitute_recipe_note_placeholders(value: Any) -> Any: """Substitute recipe note placeholders. Only strings reach this layer.""" if isinstance(value, str): - return _RECIPE_NOTE_TABLE.get(value, value) + for placeholder, note in _RECIPE_NOTE_TABLE.items(): + value = value.replace(placeholder, note) return value +def _substitute_recipe_notes_in_yaml(text: str) -> str: + """Expand shared step notes without changing the surrounding recipe source.""" + if "__RECIPE_NOTE_" not in text: + return text + root = compose_yaml(text) + if not is_yaml_mapping_node(root): + return text + steps = next((node for key, node in root.value if key.value == "steps"), None) + if not is_yaml_mapping_node(steps): + return text + edits: dict[tuple[int, int], str] = {} + for _, step in steps.value: + if not is_yaml_mapping_node(step): + continue + for field, node in step.value: + if field.value != "note": + continue + expanded = _substitute_recipe_note_placeholders(node.value) + if expanded != node.value: + start, end = node.start_mark.index, node.end_mark.index + newline = "\n" if text[start:end].endswith("\n") else "" + edits[start, end] = json.dumps(expanded, ensure_ascii=False) + newline + for (start, end), value in sorted(edits.items(), reverse=True): + text = text[:start] + value + text[end:] + return text + + _TEMP_PLACEHOLDER = "{{AUTOSKILLIT_TEMP}}" _SCRIPTS_PLACEHOLDER = "{{AUTOSKILLIT_SCRIPTS}}" diff --git a/src/autoskillit/recipe/rules/dataflow/rules_dataflow_multipart.py b/src/autoskillit/recipe/rules/dataflow/rules_dataflow_multipart.py index 397728ff1e..112a356d5a 100644 --- a/src/autoskillit/recipe/rules/dataflow/rules_dataflow_multipart.py +++ b/src/autoskillit/recipe/rules/dataflow/rules_dataflow_multipart.py @@ -25,31 +25,6 @@ def _check_multipart_iteration_notes(ctx: ValidationContext) -> list[RuleFinding if not has_multipart_step: return [] - plan_step = wf.steps.get("plan") - plan_note = (plan_step.note or "") if plan_step is not None else "" - # Also accept the glob pattern from the note of whichever step invokes the multipart skill - multipart_step_notes = [ - (step.note or "") - for step in wf.steps.values() - if step.tool in SKILL_TOOLS - and extract_skill_name(step.with_args.get("skill_command", "")) in MULTIPART_SKILL_NAMES - ] - if "*_part_*.md" not in plan_note and not any( - "*_part_*.md" in note for note in multipart_step_notes - ): - findings.append( - make_finding( - rule_name="multipart-iteration-notes", - step_name="plan", - message="Recipe uses make-plan or rectify but neither the 'plan' step note nor " - "the planning step's own note contains '*_part_*.md'. Agents will not " - "know to glob for multi-part plan files. Add: " - "'Glob plan_dir for *_part_*.md or single plan file.' to the plan " - "step's note (or to the make-plan/rectify step's note if no separate " - "'plan' step exists).", - ) - ) - sequential_keywords = ("SEQUENTIAL EXECUTION", "full cycle", "Never run verify for all parts") rules_text = " ".join(wf.kitchen_rules) if not any(kw in rules_text for kw in sequential_keywords): diff --git a/src/autoskillit/recipes/diagrams/implementation-groups.md b/src/autoskillit/recipes/diagrams/implementation-groups.md index 7dad121550..5614952a9e 100644 --- a/src/autoskillit/recipes/diagrams/implementation-groups.md +++ b/src/autoskillit/recipes/diagrams/implementation-groups.md @@ -1,4 +1,4 @@ - + ## implementation-groups diff --git a/src/autoskillit/recipes/diagrams/implementation.md b/src/autoskillit/recipes/diagrams/implementation.md index 52cd41d81c..98733d2e27 100644 --- a/src/autoskillit/recipes/diagrams/implementation.md +++ b/src/autoskillit/recipes/diagrams/implementation.md @@ -1,4 +1,4 @@ - + ## implementation diff --git a/src/autoskillit/recipes/diagrams/remediation.md b/src/autoskillit/recipes/diagrams/remediation.md index 7a3ad79f51..fe3ca00d1d 100644 --- a/src/autoskillit/recipes/diagrams/remediation.md +++ b/src/autoskillit/recipes/diagrams/remediation.md @@ -1,4 +1,4 @@ - + ## remediation diff --git a/src/autoskillit/recipes/implementation-groups.json b/src/autoskillit/recipes/implementation-groups.json index cab0d3507b..a4e9b58e9a 100644 --- a/src/autoskillit/recipes/implementation-groups.json +++ b/src/autoskillit/recipes/implementation-groups.json @@ -256,7 +256,7 @@ "on_failure": "release_issue_failure", "on_rate_limit": "release_issue_failure", "on_context_limit": "salvage_plan", - "note": "Produces plan in {{AUTOSKILLIT_TEMP}}/make-plan/. Glob plan_dir for *_part_*.md or single plan file. plan_parts context key contains the full ordered list of part files (sorted alphabetically). For a single-part plan, plan_parts has one entry equal to plan_path. GROUPS MODE: Set skill_inputs.task to the current group's file path selected from context.group_files — do NOT pass the entire list or read the file; pass the path directly to the skill. REMEDIATION MODE: on re-entry, pass the exact current audit_cycle_path; make-plan emits the matching plan_disposition_path. ACCUMULATION: After each plan step execution in groups mode, the agent MUST append the captured plan_path to context.all_plan_paths using a comma separator instead of letting the capture block overwrite the accumulated value. For multi-group runs, the accumulated value looks like \"/path/groupA_plan.md,/path/groupB_plan.md\". ISSUE CONTEXT: issue_url is passed via skill_inputs.issue_url — do NOT append it to skill_command. The make-plan skill reads skill_inputs.issue_url directly and calls fetch_github_issue internally when non-empty. ADVERSARIAL REVIEW LEVEL: adversarial_review_level is passed via skill_inputs.adversarial_review_level — do NOT append it to skill_command. The make-plan skill reads it directly from skill_inputs when non-empty. verdict=plan is the only successful result.\n" + "note": "Produces plan in {{AUTOSKILLIT_TEMP}}/make-plan/. For a single-part plan, plan_parts has one entry equal to plan_path. GROUPS MODE: Set skill_inputs.task to the current group's file path selected from context.group_files — do NOT pass the entire list or read the file; pass the path directly to the skill. REMEDIATION MODE: on re-entry, pass the exact current audit_cycle_path; make-plan emits the matching plan_disposition_path. ACCUMULATION: After each plan step execution in groups mode, the agent MUST append the captured plan_path to context.all_plan_paths using a comma separator instead of letting the capture block overwrite the accumulated value. For multi-group runs, the accumulated value looks like \"/path/groupA_plan.md,/path/groupB_plan.md\". ISSUE CONTEXT: issue_url is passed via skill_inputs.issue_url — do NOT append it to skill_command. The make-plan skill reads skill_inputs.issue_url directly and calls fetch_github_issue internally when non-empty. ADVERSARIAL REVIEW LEVEL: adversarial_review_level is passed via skill_inputs.adversarial_review_level — do NOT append it to skill_command. The make-plan skill reads it directly from skill_inputs when non-empty. verdict=plan is the only successful result." }, "salvage_plan": { "tool": "run_python", @@ -318,7 +318,8 @@ "on_failure": "release_issue_failure", "optional_context_refs": [ "plan_set_authority_path" - ] + ], + "note": "__RECIPE_NOTE_PLAN_PARTS_BINDING__ coverage_failed here is terminal by decision. Two producers (plan, gap_replan) share this bind, and a correct replan must return to the one that wrote the rejected part. Relay the result's coverage_gaps in the failure diagnostic: it names the part, the rows and the cells." }, "check_group_plan_iteration": { "tool": "run_python", @@ -529,7 +530,7 @@ } ], "on_failure": "release_issue_failure", - "note": "Rebinds current part bytes after walkthrough edits before implementation." + "note": "Rebinds current part bytes after walkthrough edits before implementation. __RECIPE_NOTE_PLAN_SET_RENEWAL_FAILURE__" }, "create_impl_worktree": { "tool": "run_cmd", diff --git a/src/autoskillit/recipes/implementation-groups.yaml b/src/autoskillit/recipes/implementation-groups.yaml index 925411eea1..de87cc3da0 100644 --- a/src/autoskillit/recipes/implementation-groups.yaml +++ b/src/autoskillit/recipes/implementation-groups.yaml @@ -254,25 +254,22 @@ steps: on_failure: release_issue_failure on_rate_limit: release_issue_failure on_context_limit: salvage_plan - note: "Produces plan in {{AUTOSKILLIT_TEMP}}/make-plan/. Glob plan_dir for *_part_*.md\ - \ or single plan file. plan_parts context key contains the full ordered list\ - \ of part files (sorted alphabetically). For a single-part plan, plan_parts\ - \ has one entry equal to plan_path. GROUPS MODE: Set skill_inputs.task to the\ - \ current group's file path selected from context.group_files \u2014 do NOT\ - \ pass the entire list or read the file; pass the path directly to the skill.\ - \ REMEDIATION MODE: on re-entry, pass the exact current audit_cycle_path; make-plan\ - \ emits the matching plan_disposition_path. ACCUMULATION: After each plan step\ - \ execution in groups mode, the agent MUST append the captured plan_path to\ - \ context.all_plan_paths using a comma separator instead of letting the capture\ - \ block overwrite the accumulated value. For multi-group runs, the accumulated\ - \ value looks like \"/path/groupA_plan.md,/path/groupB_plan.md\". ISSUE CONTEXT:\ - \ issue_url is passed via skill_inputs.issue_url \u2014 do NOT append it to skill_command.\ - \ The make-plan skill reads skill_inputs.issue_url directly and calls fetch_github_issue\ - \ internally when non-empty. ADVERSARIAL REVIEW LEVEL: adversarial_review_level\ - \ is passed via skill_inputs.adversarial_review_level \u2014 do NOT append it to\ - \ skill_command. The make-plan skill reads it directly from skill_inputs when\ - \ non-empty. verdict=plan\ - \ is the only successful result.\n" + note: >- + Produces plan in {{AUTOSKILLIT_TEMP}}/make-plan/. For a single-part plan, plan_parts has + one entry equal to plan_path. GROUPS MODE: Set skill_inputs.task to the current group's + file path selected from context.group_files — do NOT pass the entire list or read the + file; pass the path directly to the skill. REMEDIATION MODE: on re-entry, pass the exact + current audit_cycle_path; make-plan emits the matching plan_disposition_path. + ACCUMULATION: After each plan step execution in groups mode, the agent MUST append the + captured plan_path to context.all_plan_paths using a comma separator instead of letting + the capture block overwrite the accumulated value. For multi-group runs, the accumulated + value looks like "/path/groupA_plan.md,/path/groupB_plan.md". ISSUE CONTEXT: issue_url + is passed via skill_inputs.issue_url — do NOT append it to skill_command. The make-plan + skill reads skill_inputs.issue_url directly and calls fetch_github_issue internally when + non-empty. ADVERSARIAL REVIEW LEVEL: adversarial_review_level is passed via + skill_inputs.adversarial_review_level — do NOT append it to skill_command. The make-plan + skill reads it directly from skill_inputs when non-empty. verdict=plan is the only + successful result. salvage_plan: tool: run_python with: @@ -321,6 +318,12 @@ steps: on_failure: release_issue_failure optional_context_refs: - plan_set_authority_path + note: >- + __RECIPE_NOTE_PLAN_PARTS_BINDING__ + coverage_failed here is terminal by decision. Two producers (plan, gap_replan) share + this bind, and a correct replan must return to the one that wrote the rejected part. + Relay the result's coverage_gaps in the failure diagnostic: it names the part, the rows + and the cells. check_group_plan_iteration: tool: run_python with: @@ -475,7 +478,9 @@ steps: route: create_impl_worktree - route: release_issue_failure on_failure: release_issue_failure - note: Rebinds current part bytes after walkthrough edits before implementation. + note: >- + Rebinds current part bytes after walkthrough edits before implementation. + __RECIPE_NOTE_PLAN_SET_RENEWAL_FAILURE__ create_impl_worktree: tool: run_cmd with: diff --git a/src/autoskillit/recipes/implementation.json b/src/autoskillit/recipes/implementation.json index 0847f38d44..583014e08e 100644 --- a/src/autoskillit/recipes/implementation.json +++ b/src/autoskillit/recipes/implementation.json @@ -524,7 +524,7 @@ "audit_cycle_path", "plan_set_coverage_gaps" ], - "note": "Produces an ordinary plan when no audit_cycle_path is present. On NO GO re-entry, consumes that exact current authority and emits both plan_path and plan_disposition_path. all_plan_paths appends result.plan_path across re-entries; verdict=plan is the only successful result. Glob plan_dir for *_part_*.md or a single plan file and preserve alphabetical part order." + "note": "Produces an ordinary plan when no audit_cycle_path is present. On NO GO re-entry, consumes that exact current authority and emits both plan_path and plan_disposition_path. all_plan_paths appends result.plan_path across re-entries; verdict=plan is the only successful result." }, "salvage_plan": { "tool": "run_python", @@ -587,7 +587,7 @@ } ], "on_failure": "release_issue_failure", - "note": "Aggregate plan-set gate before every walkthrough.\n\nSingle-shot plans omit parent_authority_path intentionally: there is no prior\nmultipart authority to thread through. Multi-shot bind_group_parts in\nimplementation-groups.yaml supplies one because group iteration threads the\nparent digest from the previous cycle's bind step.\n" + "note": "__RECIPE_NOTE_PLAN_PARTS_BINDING__ Aggregate plan-set gate before every walkthrough. Single-shot plans omit parent_authority_path intentionally: there is no prior multipart authority to thread through. Multi-shot bind_group_parts in implementation-groups.yaml supplies one because group iteration threads the parent digest from the previous cycle's bind step." }, "check_replan_iteration": { "tool": "run_python", @@ -692,7 +692,7 @@ } ], "on_failure": "release_issue_failure", - "note": "Renews plan-set bytes after a walkthrough before implementation." + "note": "Renews plan-set bytes after a walkthrough before implementation. __RECIPE_NOTE_PLAN_SET_RENEWAL_FAILURE__" }, "create_impl_worktree": { "tool": "run_cmd", diff --git a/src/autoskillit/recipes/implementation.yaml b/src/autoskillit/recipes/implementation.yaml index 492628cce9..8192890687 100644 --- a/src/autoskillit/recipes/implementation.yaml +++ b/src/autoskillit/recipes/implementation.yaml @@ -451,11 +451,11 @@ steps: optional_context_refs: - audit_cycle_path - plan_set_coverage_gaps - note: Produces an ordinary plan when no audit_cycle_path is present. On NO GO - re-entry, consumes that exact current authority and emits both plan_path and + note: >- + Produces an ordinary plan when no audit_cycle_path is present. On NO GO re-entry, + consumes that exact current authority and emits both plan_path and plan_disposition_path. all_plan_paths appends result.plan_path across re-entries; - verdict=plan is the only successful result. Glob plan_dir for *_part_*.md or - a single plan file and preserve alphabetical part order. + verdict=plan is the only successful result. salvage_plan: tool: run_python with: @@ -503,13 +503,12 @@ steps: route: check_replan_iteration - route: release_issue_failure on_failure: release_issue_failure - note: | - Aggregate plan-set gate before every walkthrough. - - Single-shot plans omit parent_authority_path intentionally: there is no prior - multipart authority to thread through. Multi-shot bind_group_parts in - implementation-groups.yaml supplies one because group iteration threads the - parent digest from the previous cycle's bind step. + note: >- + __RECIPE_NOTE_PLAN_PARTS_BINDING__ + Aggregate plan-set gate before every walkthrough. Single-shot plans omit + parent_authority_path intentionally: there is no prior multipart authority to thread + through. Multi-shot bind_group_parts in implementation-groups.yaml supplies one because + group iteration threads the parent digest from the previous cycle's bind step. check_replan_iteration: tool: run_python with: @@ -591,7 +590,8 @@ steps: route: create_impl_worktree - route: release_issue_failure on_failure: release_issue_failure - note: Renews plan-set bytes after a walkthrough before implementation. + note: >- + Renews plan-set bytes after a walkthrough before implementation. __RECIPE_NOTE_PLAN_SET_RENEWAL_FAILURE__ create_impl_worktree: tool: run_cmd with: diff --git a/src/autoskillit/recipes/remediation.json b/src/autoskillit/recipes/remediation.json index 75d3fc48f8..bf645b685a 100644 --- a/src/autoskillit/recipes/remediation.json +++ b/src/autoskillit/recipes/remediation.json @@ -603,7 +603,7 @@ "on_failure": "release_issue_failure", "on_rate_limit": "release_issue_failure", "on_context_limit": "salvage_rectify_plan", - "note": "Glob plan_dir for *_part_*.md or single plan file. Sort alphabetically into plan_parts[].\n" + "note": "plan_parts is exactly the plan_parts value of this step's result, in the order the skill emitted it." }, "salvage_rectify_plan": { "tool": "run_python", @@ -659,7 +659,8 @@ "route": "release_issue_failure" } ], - "on_failure": "release_issue_failure" + "on_failure": "release_issue_failure", + "note": "__RECIPE_NOTE_PLAN_PARTS_BINDING__" }, "check_replan_iteration": { "tool": "run_python", @@ -766,7 +767,8 @@ "route": "release_issue_failure" } ], - "on_failure": "release_issue_failure" + "on_failure": "release_issue_failure", + "note": "__RECIPE_NOTE_PLAN_SET_RENEWAL_FAILURE__" }, "retry_walkthrough": { "tool": "run_skill", @@ -821,7 +823,7 @@ } ], "on_failure": "release_issue_failure", - "note": "Bounded one-hop re-invoke of dry-walkthrough after a context-limit stumble on the primary dry_walkthrough step. Never routes back to dry_walkthrough — on_failure and on_context_limit both fall through to the original failure destination." + "note": "Bounded one-hop re-invoke of dry-walkthrough after a context-limit stumble on the primary dry_walkthrough step. Never routes back to dry_walkthrough — on_failure and on_context_limit both fall through to the original failure destination. __RECIPE_NOTE_PLAN_SET_RENEWAL_FAILURE__" }, "create_impl_worktree": { "tool": "run_cmd", diff --git a/src/autoskillit/recipes/remediation.yaml b/src/autoskillit/recipes/remediation.yaml index 9d7dfb829b..37693bc758 100644 --- a/src/autoskillit/recipes/remediation.yaml +++ b/src/autoskillit/recipes/remediation.yaml @@ -499,10 +499,9 @@ steps: on_failure: release_issue_failure on_rate_limit: release_issue_failure on_context_limit: salvage_rectify_plan - note: 'Glob plan_dir for *_part_*.md or single plan file. Sort alphabetically - into plan_parts[]. - - ' + note: >- + plan_parts is exactly the plan_parts value of this step's result, in the order the skill + emitted it. salvage_rectify_plan: tool: run_python with: @@ -540,6 +539,8 @@ steps: route: check_replan_iteration - route: release_issue_failure on_failure: release_issue_failure + note: >- + __RECIPE_NOTE_PLAN_PARTS_BINDING__ check_replan_iteration: tool: run_python with: @@ -622,6 +623,8 @@ steps: route: retry_walkthrough - route: release_issue_failure on_failure: release_issue_failure + note: >- + __RECIPE_NOTE_PLAN_SET_RENEWAL_FAILURE__ retry_walkthrough: tool: run_skill model: '' @@ -664,10 +667,10 @@ steps: route: create_impl_worktree - route: release_issue_failure on_failure: release_issue_failure - note: "Bounded one-hop re-invoke of dry-walkthrough after a context-limit stumble\ - \ on the primary dry_walkthrough step. Never routes back to dry_walkthrough\ - \ \u2014 on_failure and on_context_limit both fall through to the original failure\ - \ destination." + note: >- + Bounded one-hop re-invoke of dry-walkthrough after a context-limit stumble on the + primary dry_walkthrough step. Never routes back to dry_walkthrough — on_failure and + on_context_limit both fall through to the original failure destination. __RECIPE_NOTE_PLAN_SET_RENEWAL_FAILURE__ create_impl_worktree: tool: run_cmd with: diff --git a/src/autoskillit/server/_plan_set_materializer.py b/src/autoskillit/server/_plan_set_materializer.py index 10883b5571..8e990a41fb 100644 --- a/src/autoskillit/server/_plan_set_materializer.py +++ b/src/autoskillit/server/_plan_set_materializer.py @@ -2,6 +2,7 @@ from __future__ import annotations +from collections.abc import Sequence from datetime import UTC, datetime from pathlib import Path @@ -11,6 +12,7 @@ PLAN_SET_AUTHORITY_ID_DOMAIN, PLAN_SET_MAX_ISSUE_BYTES, PLAN_SET_MAX_PART_BYTES, + AllocationRowDef, CoverageResultDef, CoverageStatus, GitHubFetcher, @@ -44,8 +46,10 @@ _ISSUE_NUMBER = re.compile(r"(?:#|/issues/)(\d+)(?:$|[/?#])") -def _failure(reason: PlanSetRejectReason, error: str) -> PlanSetBindResult: - return PlanSetBindResult(False, reason, error) +def _failure( + reason: PlanSetRejectReason, error: str, coverage_gaps: str = "" +) -> PlanSetBindResult: + return PlanSetBindResult(False, reason, error, coverage_gaps=coverage_gaps) def _write_or_verify(path: Path, data: bytes, allowed_root: Path) -> None: @@ -66,7 +70,11 @@ def _part_suffix(path: Path, *, only_part: bool) -> str: return "A" if only_part else path.stem.upper() -def _coverage_gaps(coverage: CoverageResultDef, requirements: tuple[RequirementDef, ...]) -> str: +def _coverage_gaps( + coverage: CoverageResultDef, + requirements: tuple[RequirementDef, ...], + parts: Sequence[PlanPartRef], +) -> str: requirement_text = {item.requirement_id: item.text for item in requirements} gaps: list[str] = [] for reason, values in ( @@ -79,9 +87,45 @@ def _coverage_gaps(coverage: CoverageResultDef, requirements: tuple[RequirementD gaps.extend( f"{item} [{reason}]: {requirement_text.get(item, '')}".rstrip() for item in values ) + locators = {part.part_key: part.locator for part in parts} + gaps.extend( + f"{key} [part_without_obligations]: {locators[key]} allocates no requirement; " + "every part needs at least one allocation row" + for key in coverage.parts_without_obligations + ) return "\n".join(gaps) +def _inspect_parts( + parts: Sequence[PlanPartRef], loaded_by_locator: dict[str, bytes] +) -> tuple[list[AllocationRowDef], list[str], list[str]]: + allocations: list[AllocationRowDef] = [] + errors: list[str] = [] + blocks: list[str] = [] + for part in parts: + if part.locator not in loaded_by_locator: + continue + try: + text = loaded_by_locator[part.locator].decode("utf-8") + rows = parse_part_allocation(text, part_key=part.part_key) + except (UnicodeDecodeError, ValueError) as exc: + errors.append(str(exc)) + details = [str(exc)] + else: + allocations.extend(rows) + defects = verify_allocation_evidence(text, rows) + errors.extend(f"{identifier}: {reason.value}" for identifier, reason, _ in defects) + details = [detail for _, _, detail in defects] + if details: + blocks.append( + f"Plan part rejected by the plan-set gate: {part.locator}\n" + + "\n".join(f"- {detail}" for detail in details) + + "\nWrite a corrected plan part to a new file in the form the planning skill's " + "plan template shows; keep the same requirements allocated." + ) + return allocations, errors, blocks + + def _result(authority: PlanSetAuthority, path: Path) -> PlanSetBindResult: coverage = authority.coverage return PlanSetBindResult( @@ -102,7 +146,7 @@ def _result(authority: PlanSetAuthority, path: Path) -> PlanSetBindResult: container_allocated=coverage.container_allocated, parts_without_obligations=coverage.parts_without_obligations, first_part_path=authority.parts[0].locator if authority.parts else "", - coverage_gaps=_coverage_gaps(coverage, authority.requirements), + coverage_gaps=_coverage_gaps(coverage, authority.requirements, authority.parts), ) @@ -354,23 +398,13 @@ def _assemble_authority( content_digest=compute_bytes_hash(data), ) ) - errors: list[str] = [] loaded_by_locator = {str(path): data for path, data in loaded} - for part in parts: - if part.locator not in loaded_by_locator: - continue - try: - text = loaded_by_locator[part.locator].decode("utf-8") - rows = parse_part_allocation(text, part_key=part.part_key) - except (UnicodeDecodeError, ValueError) as exc: - return _failure(PlanSetRejectReason.COVERAGE_FAILED, str(exc)) - allocations.extend(rows) - errors.extend( - f"{identifier}: {reason.value}" - for identifier, reason in verify_allocation_evidence(text, rows) - ) + rows, errors, blocks = _inspect_parts(parts, loaded_by_locator) + allocations.extend(rows) if errors: - return _failure(PlanSetRejectReason.COVERAGE_FAILED, "; ".join(errors)) + return _failure( + PlanSetRejectReason.COVERAGE_FAILED, "; ".join(errors), "\n\n".join(blocks) + ) requirements: tuple[RequirementDef, ...] unparsed: tuple[int, ...] @@ -421,7 +455,7 @@ def _assemble_authority( unknown=coverage.unknown, container_allocated=coverage.container_allocated, parts_without_obligations=coverage.parts_without_obligations, - coverage_gaps=_coverage_gaps(coverage, requirements), + coverage_gaps=_coverage_gaps(coverage, requirements, parts), ) authority = PlanSetAuthority.create( binding_mode=request.binding_mode, diff --git a/src/autoskillit/skill_resources/plan-part-form.md b/src/autoskillit/skill_resources/plan-part-form.md new file mode 100644 index 0000000000..50a40535bc --- /dev/null +++ b/src/autoskillit/skill_resources/plan-part-form.md @@ -0,0 +1,24 @@ +--- +id: plan-part-form +title: Plan-part authoring form +summary: Shared authoring rules for plan parts accepted by the plan-set allocation verifier. +--- +**Plan-part form.** `bind_plan_set` deterministically checks the allocation table, +resolves each row's step key and checks that its ID cell occurs in that step's text. +Follow this authoring form; the verifier's existing acceptance rules stay unchanged: +- Write each step as a level-3 heading `### Step N: title` (sub-steps + `### Step N.M: title`). Other heading levels, and a dash in place of the colon, are + not recognised. Do not repeat a step heading anywhere in the file, quoted examples + included. +- On the line under each heading, name every Requirement ID the step delivers + (`Requirements: P-1, P-2`). The ID must appear in that step's own text, which ends at + the next step heading or the next line starting with `# ` or `## `, code fences + included. +- The line `## Issue Requirement Allocation` appears once in the file, and its section + holds only the table: no sentence before, between or after the rows. +- Each row is the bare Requirement ID with no description, then `owned` or `shared`, + then exactly one step key copied from a heading (`Step 2.1`): not a list, a range, a + backticked key or a key with its title. One row per ID. +- Use the issue's own labels as IDs when it enumerates items; otherwise number your own + `P-1`, `P-2`. `owned` means this part alone delivers the item; `shared` means another + part allocates it too. diff --git a/src/autoskillit/skills_extended/dry-walkthrough/SKILL.md b/src/autoskillit/skills_extended/dry-walkthrough/SKILL.md index da3e435558..fea19f2614 100644 --- a/src/autoskillit/skills_extended/dry-walkthrough/SKILL.md +++ b/src/autoskillit/skills_extended/dry-walkthrough/SKILL.md @@ -4,6 +4,7 @@ write_paths: unrestricted uses_capabilities: [] requires_resources: - arch-constraint-catalog +- plan-part-form description: Plan validation executor. ALWAYS invoke this skill when instructed to validate or dry-walkthrough a plan. Do not read the plan or trace changes directly — use this skill first to load the validation workflow. hooks: @@ -338,6 +339,12 @@ or reconstruct any inventory, remediation, authority, or report. ### Step 5: Fix the Plan +Every edit must leave the part in the plan-part form. When a `### Step` heading cited +by an allocation row is renamed, renumbered, re-levelled or removed, update that row's +`Implementation Step` cell to the surviving key. Keep each allocated Requirement ID +in its step's text, and add no prose inside the allocation section. The recipe +re-verifies the part after this skill; a nonconforming part stops the pipeline. + For each issue found: 1. Directly edit the plan file to fix it 2. Do NOT add any "gap analysis" or "issues" sections to the plan diff --git a/src/autoskillit/skills_extended/make-plan/SKILL.md b/src/autoskillit/skills_extended/make-plan/SKILL.md index 25c3fdcc46..5d2e114334 100644 --- a/src/autoskillit/skills_extended/make-plan/SKILL.md +++ b/src/autoskillit/skills_extended/make-plan/SKILL.md @@ -1,5 +1,7 @@ --- name: make-plan +requires_resources: +- plan-part-form write_paths: - '{{AUTOSKILLIT_TEMP}}/make-plan/' uses_capabilities: @@ -68,9 +70,12 @@ Create focused, actionable implementation plans that recommend the technically b not prose flags or ambient files, activates remediation mode. Before reading any referenced artifact, verify that this authority is the server-published current `NO GO` head and that its generation, plan set, scope, part, round, parent, and audited-plan lineage match this run. -- `plan_set_coverage_gaps` (optional) — A bounded list of requirement gaps returned by a - failed sealed plan-set bind. On re-entry, allocate every listed requirement in the new - plan set; do not absorb the gaps into Part A by default. In grouped planning, the +- `plan_set_coverage_gaps` (optional) — Feedback from a failed plan-set bind, never empty + on re-entry. Requirement-gap lines such as `R2 [unassigned]: …` mean allocate every + listed requirement in the new plan set; do not absorb gaps into Part A by default. + A block starting `Plan part rejected by the plan-set gate: ` lists what the parser + rejected in that part: write a corrected replacement to a new file in the plan-part + form, keeping its requirements allocated. In grouped planning, the dedicated gap-replan route adds a group to the still-open set. In conflict-resolution examples, `{branch}` is the source PR branch and @@ -427,6 +432,9 @@ plan_parts = {path_to_part_a} plan_disposition_path = {absolute_path_when_in_remediation_mode} ``` +**Plan-part form.** Follow the shared `plan-part-form` resource when writing +each part; the template below applies those rules. + **Plan structure (single-part):** ```markdown # Implementation Plan: {Task Name} @@ -438,7 +446,14 @@ plan_disposition_path = {absolute_path_when_in_remediation_mode} {Tests to write first — should fail now, pass after implementation} ## Implementation Steps -{Ordered steps, each making one or more of the above tests pass} + +### Step 1: {step title} +Requirements: P-1 +{What this step changes; it makes one or more of the above tests pass} + +### Step 2: {step title} +Requirements: P-2 +{What this step changes} ## Verification {How to verify the implementation is correct} @@ -446,7 +461,8 @@ plan_disposition_path = {absolute_path_when_in_remediation_mode} ## Issue Requirement Allocation | Requirement ID | Allocation | Implementation Step | |---|---|---| -| {label from issue, or P-1 without enumerated items} | owned \| shared | Step {N.M} | +| P-1 | owned | Step 1 | +| P-2 | owned | Step 2 | ## Requirements Map | Requirement (from prose) | Implementation Step | @@ -467,7 +483,14 @@ plan_disposition_path = {absolute_path_when_in_remediation_mode} {Tests for THIS part only — should fail now, pass after THIS part's implementation} ## Implementation Steps -{Steps for THIS part only} + +### Step 1: {step title} +Requirements: P-1 +{What this step changes; it makes one or more of the above tests pass} + +### Step 2: {step title} +Requirements: P-2 +{What this step changes} ## Verification {How to verify THIS part's implementation is correct} @@ -475,7 +498,8 @@ plan_disposition_path = {absolute_path_when_in_remediation_mode} ## Issue Requirement Allocation | Requirement ID | Allocation | Implementation Step | |---|---|---| -| {label from issue, or P-1 without enumerated items} | owned \| shared | Step {N.M} | +| P-1 | owned | Step 1 | +| P-2 | owned | Step 2 | ## Requirements Map | Requirement (from prose) | Implementation Step | diff --git a/src/autoskillit/skills_extended/rectify/SKILL.md b/src/autoskillit/skills_extended/rectify/SKILL.md index 62c7183def..cc4bc0b57d 100644 --- a/src/autoskillit/skills_extended/rectify/SKILL.md +++ b/src/autoskillit/skills_extended/rectify/SKILL.md @@ -1,5 +1,7 @@ --- name: rectify +requires_resources: +- plan-part-form write_paths: - '{{AUTOSKILLIT_TEMP}}/rectify/' uses_capabilities: [] @@ -273,6 +275,12 @@ plan_parts = {path_to_part_a} {path_to_part_c} ``` +**Plan-part form.** Follow the shared `plan-part-form` resource when writing +each part; the template below applies those rules. + +Sub-steps are further level-3 headings (`### Step 2.1: …`); an allocation row cites +the sub-step key. + **Plan structure:** ```markdown # Rectify: {Topic} @@ -295,9 +303,11 @@ plan_parts = {path_to_part_a} ## Immunity Plan ### Step 1: Failing Tests +Requirements: P-1 {Tests that reproduce the issue and capture the gap — these must be written first} ### Step 2: Implementation +Requirements: P-1 {The architectural solution that makes this class of bug impossible or instantly caught, structured so each change makes a failing test pass} ## Verification @@ -306,5 +316,5 @@ plan_parts = {path_to_part_a} ## Issue Requirement Allocation | Requirement ID | Allocation | Implementation Step | |---|---|---| -| {label from issue, or P-1 without enumerated items} | owned \| shared | Step {N.M} | +| P-1 | owned | Step 2 | ``` diff --git a/tests/contracts/test_dry_walkthrough_arch_catalog_reference.py b/tests/contracts/test_dry_walkthrough_arch_catalog_reference.py index 7075da5f2d..8570478bf3 100644 --- a/tests/contracts/test_dry_walkthrough_arch_catalog_reference.py +++ b/tests/contracts/test_dry_walkthrough_arch_catalog_reference.py @@ -23,7 +23,10 @@ def test_dry_walkthrough_declares_arch_constraint_catalog_resource(): """The catalog is provided data, not a source-tree prose dependency.""" frontmatter = read_skill_frontmatter(_skill_path()) assert frontmatter.data is not None, frontmatter.error - assert frontmatter.data.get("requires_resources") == ["arch-constraint-catalog"], ( - "dry-walkthrough must declare requires_resources: [arch-constraint-catalog] " - "so the architectural constraint catalog is delivered with the projected skill" + assert frontmatter.data.get("requires_resources") == [ + "arch-constraint-catalog", + "plan-part-form", + ], ( + "dry-walkthrough must deliver the architectural constraint catalog and canonical " + "plan-part authoring form with the projected skill" ) diff --git a/tests/contracts/test_plan_part_contract.py b/tests/contracts/test_plan_part_contract.py new file mode 100644 index 0000000000..a46b08f348 --- /dev/null +++ b/tests/contracts/test_plan_part_contract.py @@ -0,0 +1,72 @@ +"""Plan-part templates stay compatible with the allocation verifier.""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +pytestmark = [pytest.mark.layer("contracts"), pytest.mark.medium] + +_ALLOCATION_HEADING = "## Issue Requirement Allocation" + + +def _allocation_blocks(content: str, skill_path: Path) -> list[str]: + blocks: list[str] = [] + block_lines: list[str] | None = None + for line_number, line in enumerate(content.splitlines(), start=1): + if line.startswith("```"): + if block_lines is None: + block_lines = [] + else: + if _ALLOCATION_HEADING in block_lines: + blocks.append("\n".join(block_lines)) + block_lines = None + else: + if line == _ALLOCATION_HEADING: + assert block_lines is not None, ( + f"{skill_path}:{line_number} has an allocation heading outside a fenced block" + ) + if block_lines is not None: + block_lines.append(line) + if block_lines is not None and _ALLOCATION_HEADING in block_lines: + blocks.append("\n".join(block_lines)) + return blocks + + +def test_bundled_plan_part_templates_pass_the_allocation_verifier() -> None: + from autoskillit.core import parse_part_allocation, verify_allocation_evidence + from autoskillit.workspace.skills import bundled_skills_dir, bundled_skills_extended_dir + + expected_skills: set[str] = set() + verified_skills: set[str] = set() + verified_block_count = 0 + for skills_root in (bundled_skills_dir(), bundled_skills_extended_dir()): + for skill_path in skills_root.rglob("SKILL.md"): + content = skill_path.read_text(encoding="utf-8") + if "**Plan-part form.**" in content: + expected_skills.add(skill_path.parent.name) + assert "Step {N.M} |" not in content, f"{skill_path} has a stale step placeholder" + assert r"owned \| shared" not in content, ( + f"{skill_path} escapes the allocation separator" + ) + + blocks = _allocation_blocks(content, skill_path) + for block_index, block in enumerate(blocks, start=1): + try: + rows = parse_part_allocation(block, part_key="P1") + except ValueError as exc: + pytest.fail( + f"{skill_path} allocation block {block_index} is not parseable: {exc}" + ) + assert rows, f"{skill_path} allocation block {block_index} has no rows" + defects = verify_allocation_evidence(block, rows) + assert not defects, ( + f"{skill_path} allocation block {block_index} has evidence defects: {defects}" + ) + verified_block_count += 1 + verified_skills.add(skill_path.parent.name) + + assert verified_block_count > 0, "no bundled plan-part allocation template was checked" + assert expected_skills, "no bundled plan-part authoring skill was found" + assert expected_skills <= verified_skills diff --git a/tests/core/io/test_markdown_sections.py b/tests/core/io/test_markdown_sections.py index 3057d91a96..a57f43e86d 100644 --- a/tests/core/io/test_markdown_sections.py +++ b/tests/core/io/test_markdown_sections.py @@ -4,7 +4,7 @@ import pytest -from autoskillit.core import STEP_HEADING_RE, extract_section, split_table_row +from autoskillit.core import STEP_HEADING_RE, extract_section, parse_pipe_table, split_table_row pytestmark = [pytest.mark.layer("core"), pytest.mark.small] @@ -13,10 +13,68 @@ def test_extract_section_requires_one_matching_heading() -> None: markdown = "# Title\n\n## Allocation\nbody\n\n## Next\nmore\n" assert extract_section(markdown, "Allocation") == "\nbody\n\n" - with pytest.raises(ValueError, match="exactly one"): + with pytest.raises(ValueError, match="exactly one") as missing: extract_section(markdown, "Missing") - with pytest.raises(ValueError, match="exactly one"): + assert str(missing.value).startswith("expected exactly one ## Missing section") + assert "; found 0 matching headings" in str(missing.value) + with pytest.raises(ValueError, match="exactly one") as duplicate: extract_section("## Allocation\na\n## Allocation\nb", "Allocation") + assert str(duplicate.value).startswith("expected exactly one ## Allocation section") + assert "; found 2 matching headings" in str(duplicate.value) + + +@pytest.mark.parametrize( + ("section", "offending_line", "message_part"), + ( + pytest.param( + "| Wrong | Allocation | Implementation Step |\n| --- | --- | --- |", + "| Wrong | Allocation | Implementation Step |", + "header", + id="wrong-header", + ), + pytest.param( + "| Requirement ID | Allocation | Implementation Step |\n| -- | --- | --- |", + "| -- | --- | --- |", + "separator", + id="bad-separator", + ), + pytest.param( + "| Requirement ID | Allocation | Implementation Step |\n" + "| --- | --- | --- |\nnot a pipe row", + "not a pipe row", + "pipe", + id="non-pipe-line", + ), + pytest.param( + "| Requirement ID | Allocation | Implementation Step |\n" + "| --- | --- | --- |\n| R1 | owned | Step 1.1 | extra |", + "| R1 | owned | Step 1.1 | extra |", + "3 columns", + id="four-cell-row", + ), + pytest.param( + f"| {'x' * 150} | Allocation | Implementation Step |\n| --- | --- | --- |", + f"| {'x' * 150} | Allocation | Implementation Step |", + "header", + id="long-line-is-truncated", + ), + ), +) +def test_parse_pipe_table_errors_quote_stripped_bounded_line( + section: str, offending_line: str, message_part: str +) -> None: + with pytest.raises(ValueError) as error: + parse_pipe_table( + section, + ("Requirement ID", "Allocation", "Implementation Step"), + ) + + message = str(error.value) + stripped = offending_line.strip() + assert message_part in message.lower() + assert stripped[:120] in message + if len(stripped) > 120: + assert stripped[120:] not in message def test_pipe_row_and_step_pattern_are_strict() -> None: diff --git a/tests/core/planset/test_allocation.py b/tests/core/planset/test_allocation.py index 7f1c867652..767e21f02c 100644 --- a/tests/core/planset/test_allocation.py +++ b/tests/core/planset/test_allocation.py @@ -9,37 +9,249 @@ pytestmark = [pytest.mark.layer("core"), pytest.mark.small] -def test_allocation_requires_step_that_mentions_requirement() -> None: - markdown = """### Step 1.1: implementation +def _plan_with_allocation(body: str, requirement_id: str, implementation_step: str) -> str: + return f"""{body} -The step omits the identifier. +## Issue Requirement Allocation +| Requirement ID | Allocation | Implementation Step | +| --- | --- | --- | +| {requirement_id} | owned | {implementation_step} | +""" + + +@pytest.mark.parametrize( + ("body", "requirement_id", "implementation_step", "reason", "detail_parts"), + ( + pytest.param( + "### Step 1: R1\nImplement the requirement.", + "R1", + "Step 1", + None, + (), + id="bare-id-in-step", + ), + pytest.param( + "### Step 1.1: t\nImplement R1.", + "R1", + "Step 1.1", + None, + (), + id="decimal-heading", + ), + pytest.param( + "### Step 3\nImplement R1.", + "R1", + "Step 3", + None, + (), + id="heading-without-title", + ), + pytest.param( + "1. Step 1: Implement R1.", + "R1", + "Step 1", + PlanSetRejectReason.ALLOCATION_STEP_MISSING, + ("Step 1", "no step heading", "found"), + id="numbered-list-is-not-a-heading", + ), + pytest.param( + "### Step 1: t\n\n- Step 1.1: Implement R1.", + "R1", + "Step 1.1", + PlanSetRejectReason.ALLOCATION_STEP_MISSING, + ("Step 1.1", "Step 1"), + id="listed-substep-is-not-a-heading", + ), + pytest.param( + "#### Step 1.1: R1", + "R1", + "Step 1.1", + PlanSetRejectReason.ALLOCATION_STEP_MISSING, + (), + id="four-hash-heading", + ), + pytest.param( + "### Step 1 — R1", + "R1", + "Step 1", + PlanSetRejectReason.ALLOCATION_STEP_MISSING, + (), + id="em-dash-title-separator", + ), + pytest.param( + "### Step 1.1 - R1", + "R1", + "Step 1.1", + PlanSetRejectReason.ALLOCATION_STEP_MISSING, + (), + id="hyphen-title-separator", + ), + pytest.param( + "### Step 1.1: R1\nImplement R1.\n### Step 1.2: R1\nAlso implement R1.", + "R1", + "Steps 1, 2", + PlanSetRejectReason.ALLOCATION_STEP_MISSING, + ("Steps 1, 2", "Step 1.1", "Step 1.2"), + id="step-range-cell", + ), + pytest.param( + "### Step 1.1: R1\nImplement R1.\n### Step 1.2: R1\nAlso implement R1.", + "R1", + "Step 1.1, Step 1.2", + PlanSetRejectReason.ALLOCATION_STEP_MISSING, + ("Step 1.1, Step 1.2", "Step 1.1", "Step 1.2"), + id="comma-separated-keys", + ), + pytest.param( + "### Step 1.1: R1\nImplement R1.", + "R1", + "1.1", + PlanSetRejectReason.ALLOCATION_STEP_MISSING, + ("1.1", "Step 1.1"), + id="bare-decimal-key", + ), + pytest.param( + "### Step 1.1: R1\nImplement R1.", + "R1", + "`Step 1.1`", + PlanSetRejectReason.ALLOCATION_STEP_MISSING, + ("`Step 1.1`", "Step 1.1"), + id="backticked-key", + ), + pytest.param( + "### Step 1.1: R1\nImplement R1.", + "R1", + "Step 99.9", + PlanSetRejectReason.ALLOCATION_STEP_MISSING, + ("Step 99.9", "Step 1.1"), + id="unknown-step-key", + ), + pytest.param( + "### Step 1.1: implementation\nThe step omits the identifier.", + "R1", + "Step 1.1", + PlanSetRejectReason.ALLOCATION_STEP_UNREFERENCED, + ("R1", "Step 1.1", "step's own text"), + id="step-omits-requirement-id", + ), + pytest.param( + "### Step 1.1: P-1\nImplement P-1.", + "P-1 (Add fn)", + "Step 1.1", + PlanSetRejectReason.ALLOCATION_STEP_UNREFERENCED, + ("P-1 (Add fn)", "Step 1.1"), + id="annotated-id-parentheses", + ), + pytest.param( + "### Step 1.1: P-1\nImplement P-1.", + "P-1: Add fn", + "Step 1.1", + PlanSetRejectReason.ALLOCATION_STEP_UNREFERENCED, + ("P-1: Add fn", "Step 1.1"), + id="annotated-id-colon", + ), + ), +) +def test_allocation_evidence_reports_cells_and_found_step_keys( + body: str, + requirement_id: str, + implementation_step: str, + reason: PlanSetRejectReason | None, + detail_parts: tuple[str, ...], +) -> None: + markdown = _plan_with_allocation(body, requirement_id, implementation_step) + rows = parse_part_allocation(markdown, part_key="P1") + + defects = verify_allocation_evidence(markdown, rows) + + if reason is None: + assert defects == () + return + assert len(defects) == 1 + found_id, found_reason, detail = defects[0] + assert found_id == requirement_id + assert found_reason is reason + for part in detail_parts: + assert part in detail + if "no step heading" in detail_parts: + lowered = detail.lower() + assert "no step heading" in lowered + assert "found" in lowered + + +def test_allocation_rejects_prose_before_table_with_line_context() -> None: + sentence = "The implementation plan starts below." + markdown = f"""### Step 1.1: R1 +Implement R1. ## Issue Requirement Allocation +{sentence} | Requirement ID | Allocation | Implementation Step | | --- | --- | --- | | R1 | owned | Step 1.1 | """ - rows = parse_part_allocation(markdown, part_key="P1") - assert verify_allocation_evidence(markdown, rows) == ( - ("R1", PlanSetRejectReason.ALLOCATION_STEP_UNREFERENCED), - ) - with pytest.raises(ValueError, match="exactly one"): - parse_part_allocation("# no allocation", part_key="P1") + with pytest.raises(ValueError) as error: + parse_part_allocation(markdown, part_key="P1") + assert "pipe" in str(error.value).lower() + assert sentence in str(error.value) -def test_allocation_flags_step_not_present_in_markdown() -> None: - markdown = """### Step 1.1: implementation -The step exists. +def test_allocation_rejects_escaped_pipe_and_quotes_raw_row() -> None: + raw_row = r"| R1 | owned \| shared | Step 1.1 |" + markdown = f"""### Step 1.1: R1 +Implement R1. ## Issue Requirement Allocation | Requirement ID | Allocation | Implementation Step | | --- | --- | --- | -| R1 | owned | Step 99.9 | +{raw_row} """ - rows = parse_part_allocation(markdown, part_key="P1") - assert verify_allocation_evidence(markdown, rows) == ( - ("R1", PlanSetRejectReason.ALLOCATION_STEP_MISSING), - ) + with pytest.raises(ValueError) as error: + parse_part_allocation(markdown, part_key="P1") + + assert "3 columns" in str(error.value) + assert raw_row in str(error.value) + + +@pytest.mark.parametrize( + ("row", "row_identifier"), + ( + ("| R1 | partial | Step 1.1 |", "R1"), + ("| R1 | owned | |", "R1"), + ("| | owned | Step 1.1 |", "| | owned | Step 1.1 |"), + ), +) +def test_allocation_rejects_invalid_rows_with_row_identity(row: str, row_identifier: str) -> None: + markdown = f"""### Step 1.1: R1 +Implement R1. + +## Issue Requirement Allocation +| Requirement ID | Allocation | Implementation Step | +| --- | --- | --- | +{row} +""" + + with pytest.raises(ValueError) as error: + parse_part_allocation(markdown, part_key="P1") + + assert row_identifier in str(error.value) + + +def test_allocation_rejects_duplicate_requirement_id() -> None: + markdown = """### Step 1.1: R1 +Implement R1. + +## Issue Requirement Allocation +| Requirement ID | Allocation | Implementation Step | +| --- | --- | --- | +| R1 | owned | Step 1.1 | +| R1 | shared | Step 1.1 | +""" + + with pytest.raises(ValueError) as error: + parse_part_allocation(markdown, part_key="P1") + + assert "R1" in str(error.value) diff --git a/tests/core/planset/test_coverage.py b/tests/core/planset/test_coverage.py index 376883a097..88c85c7ab9 100644 --- a/tests/core/planset/test_coverage.py +++ b/tests/core/planset/test_coverage.py @@ -9,6 +9,8 @@ from autoskillit.core import ( CoverageStatus, InventoryMode, + PlanSetBindResult, + PlanSetRejectReason, evaluate_coverage, extract_requirement_inventory, parse_part_allocation, @@ -45,3 +47,45 @@ def test_unenumerated_parts_require_planner_obligations() -> None: assert evaluate_coverage( InventoryMode.UNENUMERATED, (), {"P1": (), "P2": ()} ).parts_without_obligations == ("P1", "P2") + + +@pytest.mark.parametrize("coverage_gaps", ("", " \n\t")) +def test_coverage_failure_requires_nonblank_feedback(coverage_gaps: str) -> None: + with pytest.raises(ValueError): + PlanSetBindResult( + False, + PlanSetRejectReason.COVERAGE_FAILED, + "plan-set coverage has gaps", + coverage_gaps=coverage_gaps, + ) + + +@pytest.mark.parametrize( + ("success", "reason", "coverage_gaps"), + [ + (True, PlanSetRejectReason.NO_PARTS, ""), + (False, None, ""), + (True, None, "R2 [unassigned]"), + (False, PlanSetRejectReason.NO_PARTS, "R2 [unassigned]"), + ], +) +def test_bind_result_rejects_inconsistent_success_fields( + success: bool, reason: PlanSetRejectReason | None, coverage_gaps: str +) -> None: + with pytest.raises(ValueError): + PlanSetBindResult(success, reason, "", coverage_gaps=coverage_gaps) + + +def test_coverage_feedback_invariant_applies_only_to_coverage_failures() -> None: + failed = PlanSetBindResult( + False, + PlanSetRejectReason.COVERAGE_FAILED, + "plan-set coverage has gaps", + coverage_gaps="R2 [unassigned]", + ) + other_failure = PlanSetBindResult(False, PlanSetRejectReason.NO_PARTS, "no plan parts") + success = PlanSetBindResult(True, None, "") + + assert failed.coverage_gaps == "R2 [unassigned]" + assert other_failure.coverage_gaps == "" + assert success.coverage_gaps == "" diff --git a/tests/core/types/test_plan_set_authority.py b/tests/core/types/test_plan_set_authority.py index acc943f9a5..99b1ecc2b4 100644 --- a/tests/core/types/test_plan_set_authority.py +++ b/tests/core/types/test_plan_set_authority.py @@ -77,6 +77,14 @@ def test_create_digest_and_canonical_round_trip(tmp_path: Path) -> None: "execution generation", ), ({"binding_mode": "foreign"}, "binding_mode"), + ( + { + "coverage": CoverageResultDef( + CoverageStatus.PASS, parts_without_obligations=("P2",) + ) + }, + "unknown part key", + ), ], ) def test_invalid_authority_states_fail_closed( @@ -86,6 +94,12 @@ def test_invalid_authority_states_fail_closed( _authority(tmp_path, **overrides) +@pytest.mark.parametrize("mode", (InventoryMode.NO_ISSUE, InventoryMode.UNENUMERATED)) +def test_authority_rejects_omitted_empty_parts(tmp_path: Path, mode: InventoryMode) -> None: + with pytest.raises(ValueError, match="must match actual empty parts"): + _authority(tmp_path, inventory_mode=mode, allocations=()) + + def test_part_ordinals_and_resolved_locators_are_unique(tmp_path: Path) -> None: authority = _authority(tmp_path) first = authority.parts[0] @@ -101,6 +115,8 @@ def test_part_ordinals_and_resolved_locators_are_unique(tmp_path: Path) -> None: valid = _authority( tmp_path, parts=(first, replace(alias, locator=str(second))), + allocations=authority.allocations + + (replace(authority.allocations[0], requirement_id="P-2", part_key="P2"),), ) assert [part.part_suffix for part in valid.parts] == ["A", "A"] diff --git a/tests/infra/test_canonical_json_producer_convention.py b/tests/infra/test_canonical_json_producer_convention.py index 0038c812a7..5d9ed71ada 100644 --- a/tests/infra/test_canonical_json_producer_convention.py +++ b/tests/infra/test_canonical_json_producer_convention.py @@ -132,7 +132,7 @@ def _scan_child_tool_authority_producer_sites() -> set[tuple[str, int, str]]: producer_symbol="write_audit_disposition_bundle", producer_path=_TYPED_PRODUCER_MODULE, producer_function="_write_disposition_report", - skill_md_refs=(("src/autoskillit/skills_extended/make-plan/SKILL.md", 383, 395),), + skill_md_refs=(("src/autoskillit/skills_extended/make-plan/SKILL.md", 386, 398),), ), "inventory": CanonicalArtifactDef( consumer_site=("src/autoskillit/core/audit/audit_cycle_verifier.py", (660,)), @@ -146,7 +146,7 @@ def _scan_child_tool_authority_producer_sites() -> set[tuple[str, int, str]]: producer_symbol="write_audit_disposition_bundle", producer_path=_TYPED_PRODUCER_MODULE, producer_function="_write_plan_association", - skill_md_refs=(("src/autoskillit/skills_extended/make-plan/SKILL.md", 383, 395),), + skill_md_refs=(("src/autoskillit/skills_extended/make-plan/SKILL.md", 386, 398),), ), "audit_semantic_result": CanonicalArtifactDef( consumer_site=("src/autoskillit/core/audit/audit_semantic_codec.py", 423), diff --git a/tests/infra/test_e2e_harness.py b/tests/infra/test_e2e_harness.py index 121a2c9a86..7c9c671567 100644 --- a/tests/infra/test_e2e_harness.py +++ b/tests/infra/test_e2e_harness.py @@ -25,6 +25,206 @@ harness = load_check_script("_autoskillit_e2e_harness", _HARNESS_SCRIPT) e2e_catalog = harness.e2e_catalog + +@pytest.fixture(autouse=True) +def _isolated_sandbox(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(harness, "SANDBOX_CLONE", tmp_path / "workspace" / "sandbox") + + +@pytest.fixture +def failure_sources(tmp_path: Path) -> dict[Path, Path]: + runs = harness.SANDBOX_CLONE.parent / harness.PIPELINE_RUNS_DIR + project = tmp_path / "home" / ".claude" / "projects" / "project" + sources = { + runs / "clone-a/.autoskillit/temp/make-plan/a_plan.md": Path( + "pipeline-artifacts/clone-a/make-plan/a_plan.md" + ), + runs / "clone-b/.autoskillit/temp/rectify/b_plan.md": Path( + "pipeline-artifacts/clone-b/rectify/b_plan.md" + ), + runs / "clone-a/.autoskillit/temp/make-plan/nested/metadata.json": Path( + "pipeline-artifacts/clone-a/make-plan/nested/metadata.json" + ), + project / "session.jsonl": Path("transcripts/project/session.jsonl"), + project / "session/subagents/agent-one.jsonl": Path( + "transcripts/project/session/subagents/agent-one.jsonl" + ), + } + for source, relative in sources.items(): + source.parent.mkdir(parents=True, exist_ok=True) + source.write_bytes(f"evidence for {relative}\n".encode()) + (project / "ignored.txt").write_text("not a transcript") + return sources + + +@pytest.mark.parametrize( + ("case", "outcome", "passed"), + ( + ("failed", "failed", False), + ("exception", "failed", False), + ("expected_failure", "expected_failure", False), + ("passed", "passed", True), + ), +) +def test_run_test_retains_evidence_only_when_it_does_not_pass( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, failure_sources, case, outcome, passed +) -> None: + def run(*_args, **_kwargs): + if case == "exception": + raise RuntimeError("harness crashed") + return ( + ["test failed"] if case == "failed" else [], + [_SMOKE_FAILURE] if case == "expected_failure" else [], + ) + + monkeypatch.setattr(harness, "_run_test", run) + failures, result = _run_recipe(tmp_path, FakeRunner(lambda _argv: None)) + out = tmp_path / "out" + assert result["passed"] is passed + assert result["outcome"] == outcome + assert result["failures"] == failures + if passed: + assert "evidence" not in result + assert not (out / "pipeline-artifacts").exists() + assert not (out / "transcripts").exists() + return + assert result["evidence"] == {"plan_files": 3, "transcripts": 2, "errors": []} + for source, relative in failure_sources.items(): + assert (out / relative).read_bytes() == source.read_bytes() + assert not (out / "transcripts/project/ignored.txt").exists() + + +@pytest.mark.parametrize("failure_site", ("copy", "walk")) +def test_evidence_errors_preserve_the_verdict_and_continue_other_sources( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, failure_sources, failure_site +) -> None: + source = next(path for path in failure_sources if path.name == "a_plan.md") + target = source if failure_site == "copy" else source.parent + owner = harness.shutil if failure_site == "copy" else harness.os + name = "copyfile" if failure_site == "copy" else "scandir" + original = getattr(owner, name) + + def unreadable(path, *args, **kwargs): + if path in (target, str(target)): + raise OSError("cannot read evidence") + return original(path, *args, **kwargs) + + monkeypatch.setattr(owner, name, unreadable) + monkeypatch.setattr(harness, "_run_test", lambda *_args, **_kwargs: (["test failed"], [])) + failures, result = _run_recipe(tmp_path, FakeRunner(lambda _argv: None)) + assert failures == ["test failed"] + assert result["outcome"] == "failed" + assert result["passed"] is False + assert result["evidence"]["transcripts"] == 2 + assert any(str(target) in error for error in result["evidence"]["errors"]) + readable = next(path for path in failure_sources if path.name == "b_plan.md") + assert (tmp_path / "out" / failure_sources[readable]).read_bytes() == readable.read_bytes() + + +def test_unexpected_collection_error_still_writes_the_result( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + def crash(*_args, **_kwargs): + try: + raise ValueError("collection root cause") + except ValueError as exc: + raise RuntimeError("collection crashed") from exc + + monkeypatch.setattr(harness, "collect_failure_evidence", crash) + monkeypatch.setattr(harness, "_run_test", lambda *_args, **_kwargs: (["test failed"], [])) + failures, result = _run_recipe(tmp_path, FakeRunner(lambda _argv: None)) + assert failures == ["test failed"] + assert result["outcome"] == "failed" + assert result["evidence"]["plan_files"] == result["evidence"]["transcripts"] == 0 + assert "collection crashed" in result["evidence"]["errors"][0] + assert "ValueError: collection root cause" in result["evidence"]["exception"] + assert "RuntimeError: collection crashed" in result["evidence"]["exception"] + assert 'raise RuntimeError("collection crashed") from exc' in result["evidence"]["exception"] + + +def test_nothing_to_collect_adds_no_evidence_key( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setattr(harness, "_run_test", lambda *_args, **_kwargs: (["test failed"], [])) + _, result = _run_recipe(tmp_path, FakeRunner(lambda _argv: None)) + assert "evidence" not in result + + +def test_failure_evidence_skips_symlinks_and_linked_scan_ancestors(tmp_path: Path) -> None: + runs = harness.SANDBOX_CLONE.parent / harness.PIPELINE_RUNS_DIR + plans = runs / "clone-a/.autoskillit/temp/make-plan" + plans.mkdir(parents=True) + (plans / "kept.md").write_text("kept") + external = tmp_path / "external" + (external / "make-plan").mkdir(parents=True) + (external / "make-plan" / "outside.md").write_text("outside") + (plans / "file-link.md").symlink_to(external / "make-plan" / "outside.md") + (plans / "dir-link").symlink_to(external, target_is_directory=True) + ancestor = runs / "clone-b/.autoskillit" + ancestor.mkdir(parents=True) + (ancestor / "temp").symlink_to(external, target_is_directory=True) + (runs / "linked-clone").symlink_to(runs / "clone-a", target_is_directory=True) + home = tmp_path / "home" + home.mkdir() + (external / "projects").mkdir() + (external / "projects" / "outside.jsonl").write_text("outside transcript") + (home / ".claude").symlink_to(external, target_is_directory=True) + out = tmp_path / "out" + + result = harness.collect_failure_evidence(out, home) + + assert result == {"plan_files": 1, "transcripts": 0, "errors": []} + assert [path.relative_to(out) for path in out.rglob("*") if path.is_file()] == [ + Path("pipeline-artifacts/clone-a/make-plan/kept.md") + ] + + +def test_collected_plans_and_transcripts_are_redacted( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, failure_sources +) -> None: + plan = next(path for path in failure_sources if path.name == "a_plan.md") + transcript = next(path for path in failure_sources if path.name == "session.jsonl") + plan.write_bytes(MINIMAX_KEY.encode()) + transcript.write_bytes(SANDBOX_TOKEN.encode()) + monkeypatch.setattr(harness, "_run_test", lambda *_args, **_kwargs: (["test failed"], [])) + _run_recipe(tmp_path, FakeRunner(lambda _argv: None)) + upload = tmp_path / "upload" + + harness.redact_tree([tmp_path / "out"], upload, [MINIMAX_KEY, SANDBOX_TOKEN]) + + for source, secret in ((plan, MINIMAX_KEY), (transcript, SANDBOX_TOKEN)): + copied = (upload / "out" / failure_sources[source]).read_bytes() + assert harness.REDACTED in copied + assert secret.encode() not in copied + + +def test_runs_scan_error_keeps_readable_transcripts( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, failure_sources +) -> None: + runs = harness.SANDBOX_CLONE.parent / harness.PIPELINE_RUNS_DIR + original = Path.iterdir + + def unreadable(path): + if path == runs: + raise OSError("cannot enumerate runs") + return original(path) + + monkeypatch.setattr(Path, "iterdir", unreadable) + monkeypatch.setattr(harness, "_run_test", lambda *_args, **_kwargs: (["test failed"], [])) + failures, result = _run_recipe(tmp_path, FakeRunner(lambda _argv: None)) + assert failures == ["test failed"] + assert result["outcome"] == "failed" + assert result["evidence"]["plan_files"] == 0 + assert result["evidence"]["transcripts"] == 2 + assert str(runs) in result["evidence"]["errors"][0] + + +def test_pipeline_runs_directory_matches_clone_source_of_truth() -> None: + from autoskillit.workspace.clone._detect import RUNS_DIR + + assert harness.PIPELINE_RUNS_DIR == RUNS_DIR + + MINIMAX_KEY = "minimax-test-key" SANDBOX_TOKEN = "sandbox-test-token" SANDBOX = "owner/sandbox" @@ -1416,7 +1616,7 @@ def test_steps_run_in_order_with_the_token_only_on_stdin(self, tmp_path: Path) - ], ["gh", "auth", "login", "--hostname", "github.com", "--with-token"], ["gh", "auth", "setup-git"], - ["gh", "repo", "clone", SANDBOX, "/workspace/sandbox"], + ["gh", "repo", "clone", SANDBOX, str(harness.SANDBOX_CLONE)], ["gh", "pr", "list", "--repo", SANDBOX, "--state", "all"] + ["--limit", "1", "--json", "number"], ["autoskillit", "fleet", "run", "implementation", "-i", "task=Add a greeting"] @@ -1816,6 +2016,7 @@ def test_stale_expectation_fails_and_unexpected_diagnostic_is_unlisted( def test_cli_names_expected_failure_and_run_test_handles_exceptions( self, tmp_path, monkeypatch, capsys ): + monkeypatch.setattr(harness.Path, "home", classmethod(lambda _cls: tmp_path / "home")) diagnostic = _diagnostic("error", "runtime", "known clean-install failure") expected = {**diagnostic, "issue": _CLEAN_INSTALL_ISSUE} catalog_path = tmp_path / "catalog.json" diff --git a/tests/infra/test_pretty_output_recipe.py b/tests/infra/test_pretty_output_recipe.py index 02fe9a80bc..4411d10b42 100644 --- a/tests/infra/test_pretty_output_recipe.py +++ b/tests/infra/test_pretty_output_recipe.py @@ -1058,7 +1058,7 @@ def _assert_compaction_semantics(recipe_name: str, mode_name: str, loaded_result if recipe_name != "remediation": return False rectify_note = original_parsed["steps"]["rectify"]["note"] - assert "Glob plan_dir for *_part_*.md" in rectify_note, ( + assert "plan_parts is exactly the plan_parts value" in rectify_note, ( f"property=remediation rectify note source; recipe={recipe_name}; mode={mode_name}" ) assert compacted_parsed["steps"]["rectify"]["note"] == rectify_note, ( @@ -1350,9 +1350,9 @@ def test_pretty_output_recipe_grid_preserves_semantics_and_budgets(tmp_path, mon f"ingredients_only=both; actual={measured_modes!r}; limit=all registry modes" ) assert maxima == { - "get_recipe_section": (186_838, "remediation", "all_truthy"), - "load_recipe": (186_838, "remediation", "all_truthy"), - "open_kitchen": (186_892, "remediation", "all_truthy"), + "get_recipe_section": (188_124, "remediation", "all_truthy"), + "load_recipe": (188_124, "remediation", "all_truthy"), + "open_kitchen": (188_178, "remediation", "all_truthy"), }, ( "property=canonical baseline maxima; recipe=all; mode=all; tool=all; " f"ingredients_only=both; actual={maxima!r}; limit=measured current baseline" diff --git a/tests/recipe/test_bundled_recipes_pipeline_structure.py b/tests/recipe/test_bundled_recipes_pipeline_structure.py index 2ab38aff1b..8a34c2d94f 100644 --- a/tests/recipe/test_bundled_recipes_pipeline_structure.py +++ b/tests/recipe/test_bundled_recipes_pipeline_structure.py @@ -7,6 +7,8 @@ from __future__ import annotations +from pathlib import Path + import pytest from autoskillit.core.types import Severity @@ -16,6 +18,91 @@ pytestmark = [pytest.mark.layer("recipe"), pytest.mark.small] +_TERMINAL_COVERAGE_BINDS = { + ("implementation-groups", "bind_group_parts"), + ("implementation-groups", "renew_plan_set"), + ("implementation", "renew_plan_set"), + ("remediation", "renew_plan_set"), + ("remediation", "renew_before_retry"), +} + + +def _assert_bind_coverage_disposition(recipe, step_name: str) -> None: + bind = recipe.steps[step_name] + assert bind.on_result is not None + coverage_routes = [ + condition + for condition in bind.on_result.conditions + if "result.reason" in (condition.when or "") + and "coverage_failed" in (condition.when or "") + ] + if (recipe.name, step_name) in _TERMINAL_COVERAGE_BINDS: + assert not coverage_routes + return + assert len(coverage_routes) == 1, f"undecided bind site: {recipe.name}/{step_name}" + assert bind.capture["plan_set_coverage_gaps"].from_ == "${{ result.coverage_gaps }}" + guard = recipe.steps[coverage_routes[0].route] + assert guard.tool == "run_python" + assert guard.with_args["callable"] == "autoskillit.smoke_utils.check_loop_iteration" + assert guard.with_args["max_iterations"] == "2" + assert guard.on_result is not None + routes = [condition.route for condition in guard.on_result.conditions if not condition.when] + assert len(routes) == 1 + replan_name = routes[0] + replan = recipe.steps[replan_name] + assert replan.tool == "run_skill" + inputs = replan.with_args["skill_inputs"] + assert isinstance(inputs, dict) + assert "${{ context.plan_set_coverage_gaps }}" in inputs.values() + assert any( + recipe.steps[target].tool == "bind_plan_set" + for target in _build_success_step_graph(recipe)[replan_name] + ) + + +def test_all_bind_sites_have_decided_coverage_failure_disposition() -> None: + bind_sites: set[tuple[str, str]] = set() + for path in sorted(builtin_recipes_dir().glob("*.yaml")): + recipe = load_recipe(path) + for step_name, step in recipe.steps.items(): + if step.tool == "bind_plan_set": + bind_sites.add((recipe.name, step_name)) + _assert_bind_coverage_disposition(recipe, step_name) + assert _TERMINAL_COVERAGE_BINDS <= bind_sites + + +def test_bind_disposition_guard_names_an_undecided_bind(tmp_path: Path) -> None: + source = (builtin_recipes_dir() / "implementation.yaml").read_text() + scratch = tmp_path / "implementation.yaml" + scratch.write_text( + source.replace( + "\nsteps:\n", + "\nsteps:\n undecided_bind:\n tool: bind_plan_set\n" + " on_result:\n - route: release_issue_failure\n", + 1, + ) + ) + recipe = load_recipe(scratch) + with pytest.raises(AssertionError, match="undecided bind site: implementation/undecided_bind"): + _assert_bind_coverage_disposition(recipe, "undecided_bind") + + +def test_binds_use_the_producer_part_list_without_directory_discovery() -> None: + for path in sorted(builtin_recipes_dir().glob("*.yaml")): + recipe = load_recipe(path) + binds = {name for name, step in recipe.steps.items() if step.tool == "bind_plan_set"} + if binds: + assert all("*_part_*.md" not in (step.note or "") for step in recipe.steps.values()) + graph = _build_success_step_graph(recipe) + for bind_name in binds: + if recipe.steps[bind_name].with_args.get("plan_parts") != "${{ context.plan_parts }}": + continue + for source, targets in graph.items(): + if bind_name in targets: + assert recipe.steps[source].capture_list["plan_parts"].from_ == ( + "${{ result.plan_parts }}" + ), f"{recipe.name}/{source} must capture the replacement list for {bind_name}" + def _skill_input(step, name: str) -> object: values = step.with_args.get("skill_inputs", {}) @@ -326,14 +413,6 @@ def test_ip5_audit_impl_has_on_failure(self, recipe) -> None: assert step.on_success is None # on_result is used; on_success remains absent assert step.on_failure == "check_audit_integrity_retry" - def test_ip6_plan_step_note_contains_glob_pattern(self, recipe) -> None: - """T_IP6: plan step note must contain *_part_*.md glob pattern for multi-part discovery.""" - note = recipe.steps["plan"].note or "" - assert "*_part_*.md" in note, ( - "plan step note must contain glob pattern for multi-part discovery; " - "if removed, agents will not discover part files" - ) - def test_ip7_verify_step_note_sequential_constraint(self, recipe) -> None: """T_IP7: verify step note must contain sequential execution constraint.""" note = recipe.steps["verify"].note or "" @@ -647,7 +726,13 @@ def test_ip_next_or_done_no_more_groups_route(self, recipe) -> None: def test_ip_coverage_replan_is_bounded(self, recipe) -> None: bind = recipe.steps["bind_plan_set"] assert bind.on_result is not None - assert any(c.route == "check_replan_iteration" for c in bind.on_result.conditions) + assert any( + c.route == "check_replan_iteration" + and c.when + and "result.reason" in c.when + and "coverage_failed" in c.when + for c in bind.on_result.conditions + ) guard = recipe.steps["check_replan_iteration"] assert guard.with_args["max_iterations"] == "2" assert guard.on_result is not None @@ -712,7 +797,13 @@ def test_ig6_next_or_done_routes_more_parts_to_review(self, recipe) -> None: def test_ig_gap_replan_is_bounded(self, recipe) -> None: seal = recipe.steps["seal_plan_set"] assert seal.on_result is not None - assert any(c.route == "check_replan_iteration" for c in seal.on_result.conditions) + assert any( + c.route == "check_replan_iteration" + and c.when + and "result.reason" in c.when + and "coverage_failed" in c.when + for c in seal.on_result.conditions + ) guard = recipe.steps["check_replan_iteration"] assert guard.with_args["max_iterations"] == "2" assert guard.on_result is not None diff --git a/tests/recipe/test_implementation.py b/tests/recipe/test_implementation.py index 3e8f4c2f01..1b2aeb3d6f 100644 --- a/tests/recipe/test_implementation.py +++ b/tests/recipe/test_implementation.py @@ -21,7 +21,7 @@ RECIPE_PATH = builtin_recipes_dir() / "implementation.yaml" _PRE_DELIVERY_STRUCTURE_SHA256 = ( - "sha256:03057194113eef6e9902d5b26ed974c8da839a66f17aa8b20f70b8f8ebdbb375" + "sha256:73734dbf63321cb0ee4d72e28101fa29f1f51dbbb8a8064df96552958b08a9f6" ) _CHECKPOINT_HANDLER_TABLE = { diff --git a/tests/recipe/test_io_json_precompile.py b/tests/recipe/test_io_json_precompile.py index 374f95b293..1f9c514681 100644 --- a/tests/recipe/test_io_json_precompile.py +++ b/tests/recipe/test_io_json_precompile.py @@ -56,6 +56,53 @@ def _compile_json(yaml_path: Path) -> None: json_path.write_text(json.dumps(data, indent=2, ensure_ascii=False) + "\n", encoding="utf-8") +def test_load_recipe_dict_expands_shared_notes_in_surrounding_text(tmp_path: Path) -> None: + yaml_path = tmp_path / "recipe.yaml" + _write_yaml( + yaml_path, + { + **_MINIMAL_RECIPE, + "steps": { + "done": { + "action": "stop", + "note": "Before. __RECIPE_NOTE_PLAN_SET_RENEWAL_FAILURE__ After.", + } + }, + }, + ) + _compile_json(yaml_path) + note = load_recipe_dict(yaml_path)["steps"]["done"]["note"] + assert note.startswith("Before. coverage_failed here is terminal by decision.") + assert note.endswith("it names the rows the edit broke. After.") + assert "__RECIPE_NOTE_" not in note + + +def test_raw_recipe_delivery_expands_shared_notes_as_valid_yaml(tmp_path: Path) -> None: + from autoskillit.recipe import load_and_validate + + yaml_path = tmp_path / ".autoskillit" / "recipes" / "test-recipe.yaml" + yaml_path.parent.mkdir(parents=True) + _write_yaml( + yaml_path, + { + **_MINIMAL_RECIPE, + "steps": { + "done": { + "action": "stop", + "message": "Done.", + "note": "Before. __RECIPE_NOTE_PLAN_SET_RENEWAL_FAILURE__ After.", + } + }, + }, + ) + delivered = load_and_validate("test-recipe", project_dir=tmp_path, temp_dir=tmp_path) + assert "__RECIPE_NOTE_" not in delivered["content"] + assert ( + load_yaml(delivered["content"])["steps"]["done"]["note"] + == (load_recipe_dict(yaml_path)["steps"]["done"]["note"]) + ) + + def test_load_recipe_dict_prefers_json_when_fresh(tmp_path, monkeypatch): yaml_path = tmp_path / "recipe.yaml" _write_yaml(yaml_path, _MINIMAL_RECIPE) diff --git a/tests/recipe/test_remediation_recipe.py b/tests/recipe/test_remediation_recipe.py index cfd31877e6..05fdce9211 100644 --- a/tests/recipe/test_remediation_recipe.py +++ b/tests/recipe/test_remediation_recipe.py @@ -22,7 +22,7 @@ Path(__file__).parent.parent.parent / "src" / "autoskillit" / "recipes" / "remediation.yaml" ) _PRE_DELIVERY_STRUCTURE_SHA256 = ( - "sha256:ecd6705d9e08441738c914d0853a975c292bdd341e1bf7957e35256d4ca11777" + "sha256:64e553d7e7669f32968ac401161139869fce71c023dbb129ba977d0dd2e3d831" ) @@ -48,7 +48,13 @@ def test_context_limited_walkthrough_renews_before_retry(recipe) -> None: def test_coverage_replan_is_bounded(recipe) -> None: bind = recipe.steps["bind_plan_set"] assert bind.on_result is not None - assert any(c.route == "check_replan_iteration" for c in bind.on_result.conditions) + assert any( + c.route == "check_replan_iteration" + and c.when + and "result.reason" in c.when + and "coverage_failed" in c.when + for c in bind.on_result.conditions + ) guard = recipe.steps["check_replan_iteration"] assert guard.with_args["max_iterations"] == "2" assert guard.on_result is not None diff --git a/tests/recipe/test_rules_multipart_iteration.py b/tests/recipe/test_rules_multipart_iteration.py index 08e325887d..bd2970132e 100644 --- a/tests/recipe/test_rules_multipart_iteration.py +++ b/tests/recipe/test_rules_multipart_iteration.py @@ -9,32 +9,6 @@ class TestMultipartIterationRule: - def test_mi1_multipart_rule_warns_on_missing_glob_note(self) -> None: - """T_MI1: multipart-glob-note fires when make-plan step has no *_part_*.md in note.""" - recipe = Recipe( - name="test-recipe", - description="test", - ingredients={}, - steps={ - "plan": RecipeStep( - tool="run_skill", - with_args={"skill_command": "/autoskillit:make-plan inputs.task"}, - on_success="verify", - note="Produces a plan file.", - ), - "verify": RecipeStep( - tool="run_skill", - with_args={"skill_command": "/autoskillit:dry-walkthrough context.plan_path"}, - on_success="done", - ), - "done": RecipeStep(action="stop", message="Done"), - }, - kitchen_rules=[], - ) - warnings = run_semantic_rules(recipe) - rule_names = [w.rule for w in warnings] - assert "multipart-iteration-notes" in rule_names - def test_mi2_multipart_rule_passes_compliant_recipe(self) -> None: """T_MI2: Validator emits no multipart warnings when all conventions are present.""" recipe = Recipe( @@ -46,7 +20,7 @@ def test_mi2_multipart_rule_passes_compliant_recipe(self) -> None: tool="run_skill", with_args={"skill_command": "/autoskillit:make-plan inputs.task"}, on_success="verify", - note="Glob plan_dir for *_part_*.md or single plan file.", + note="Produces the plan_parts result.", ), "verify": RecipeStep( tool="run_skill", @@ -78,7 +52,7 @@ def test_mi3_multipart_rule_passes_non_verify_backroute(self) -> None: tool="run_skill", with_args={"skill_command": "/autoskillit:rectify context.investigation_path"}, on_success="dry_walkthrough", - note="Glob plan_dir for *_part_*.md or single plan file.", + note="Produces the plan_parts result.", ), "dry_walkthrough": RecipeStep( tool="run_skill", @@ -111,7 +85,7 @@ def test_mi4_multipart_rule_fires_when_no_more_parts_route(self) -> None: tool="run_skill", with_args={"skill_command": "/autoskillit:make-plan inputs.task"}, on_success="next_or_done", - note="Glob plan_dir for *_part_*.md or single plan file.", + note="Produces the plan_parts result.", ), "next_or_done": RecipeStep( action="route", @@ -138,7 +112,7 @@ def compliant_multipart_recipe_no_list() -> Recipe: tool="run_skill", with_args={"skill_command": "/autoskillit:make-plan inputs.task"}, capture={"plan_path": "${{ result.plan_path }}"}, - note="Glob plan_dir for *_part_*.md or single plan file. Sort into plan_parts[].", + note="Produces the plan_parts result.", on_success="done", ), "done": RecipeStep(action="stop", message="Done"), @@ -161,7 +135,7 @@ def compliant_multipart_recipe_with_list() -> Recipe: capture={"plan_path": "${{ result.plan_path }}"}, capture_list={"plan_parts": "${{ result.plan_parts }}"}, retries=0, - note="Glob plan_dir for *_part_*.md or single plan file. Sort into plan_parts[].", + note="Produces the plan_parts result.", on_success="done", ), "done": RecipeStep(action="stop", message="Done"), diff --git a/tests/server/test_delivery_e2e_verification.py b/tests/server/test_delivery_e2e_verification.py index e346a0380e..0211538777 100644 --- a/tests/server/test_delivery_e2e_verification.py +++ b/tests/server/test_delivery_e2e_verification.py @@ -251,9 +251,9 @@ async def test_implementation_bounded_path_counts_automatic_and_advertised_deliv # Pin normalized delivery costs to detect drift in the bundled recipe. # These measured totals are not production-safe thresholds. assert totals == { - "raw_chars": 18_551, - "utf8_bytes": 18_553, - "client_serialized_chars": 20_634, - "estimated_tokens": 4_636, + "raw_chars": 18_800, + "utf8_bytes": 18_802, + "client_serialized_chars": 20_871, + "estimated_tokens": 4_698, "responses": 5, } diff --git a/tests/server/test_plan_set_materializer.py b/tests/server/test_plan_set_materializer.py index a6e76181fe..a6224ff7b0 100644 --- a/tests/server/test_plan_set_materializer.py +++ b/tests/server/test_plan_set_materializer.py @@ -30,6 +30,23 @@ async def fetch_issue(self, *_args: object, **_kwargs: object) -> dict[str, obje } +def _plan_part( + allocation_rows: str, + *, + before_table: str = "", + steps: str = "### Step 1.1: R1\nImplement R1.", +) -> str: + return f"""# Plan part +{steps} + +## Issue Requirement Allocation +{before_table} +| Requirement ID | Allocation | Implementation Step | +| --- | --- | --- | +{allocation_rows} +""" + + @pytest.mark.asyncio async def test_sealed_binding_writes_content_addressed_authority(tmp_path: Path) -> None: fixture = _FIXTURES / "split_r1_r2" @@ -55,6 +72,120 @@ async def test_sealed_binding_writes_content_addressed_authority(tmp_path: Path) assert Path(result.plan_set_authority_path).is_file() +def _coverage_failure_case( + case: str, +) -> tuple[tuple[str, ...], str, tuple[str, ...], tuple[str, ...], tuple[str, ...]]: + expected_gaps: tuple[str, ...] + expected_errors: tuple[str, ...] = () + expected_forms: tuple[str, ...] = () + + if case == "evidence": + part_texts = ( + _plan_part( + "| R1 | owned | Steps 1, 2 |\n| R2 (Add fn) | owned | Step 1.2 |", + steps=("### Step 1.1: R1\nImplement R1.\n\n### Step 1.2: R2\nImplement R2."), + ), + ) + issue_url = "owner/repo#1" + expected_gaps = ( + "R1", + "R2 (Add fn)", + "Steps 1, 2", + "Step 1.1", + "Step 1.2", + ) + expected_errors = ("allocation_step_missing", "allocation_step_unreferenced") + expected_forms = ("Expected exactly one key such as Step 2.1",) + elif case == "table": + sentence = "The plan explanation appears before the allocation table." + part_texts = (_plan_part("| R1 | owned | Step 1.1 |", before_table=sentence),) + issue_url = "owner/repo#1" + expected_gaps = (sentence,) + expected_forms = ("table rows must be pipe-delimited",) + elif case == "row": + part_texts = (_plan_part("| R1 | partial | Step 1.1 |"),) + issue_url = "owner/repo#1" + expected_gaps = ("R1", "partial") + expected_forms = ("use owned or shared",) + elif case == "two_parts": + part_texts = ( + _plan_part( + "| R1 | owned | Step 1.1 |", + before_table="The first part has prose before its table.", + ), + _plan_part("| R2 | partial | Step 1.1 |"), + ) + issue_url = "owner/repo#1" + expected_gaps = ( + "The first part has prose before its table.", + "R2", + "partial", + ) + expected_forms = ("table rows must be pipe-delimited", "use owned or shared") + elif case == "part_without_obligations": + part_texts = (_plan_part(""),) + issue_url = "" + expected_gaps = ("part_without_obligations",) + else: + fixture = _FIXTURES / "missing_r2" + part_texts = tuple( + (fixture / name).read_text(encoding="utf-8") for name in ("part_a.md", "part_b.md") + ) + issue_url = "owner/repo#1" + expected_gaps = ("R2 [unassigned]",) + + return part_texts, issue_url, expected_gaps, expected_errors, expected_forms + + +@pytest.mark.parametrize( + "case", + ("evidence", "table", "row", "two_parts", "part_without_obligations", "unassigned"), +) +@pytest.mark.asyncio +async def test_coverage_failures_return_feedback(tmp_path: Path, case: str) -> None: + part_texts, issue_url, expected_gaps, expected_errors, expected_forms = _coverage_failure_case( + case + ) + + part_names = ( + ("part_a.md", "part_b.md") + if case == "unassigned" + else tuple(f"{case}_part_{index}.md" for index in range(1, len(part_texts) + 1)) + ) + part_paths = tuple(tmp_path / name for name in part_names) + for path, text in zip(part_paths, part_texts, strict=True): + path.write_text(text, encoding="utf-8") + request = PlanSetBindRequest( + plan_parts_raw="\n".join(str(path) for path in part_paths), + allowed_root=tmp_path, + issue_url=issue_url, + parent_authority_path="", + seal=True, + step_name="bind", + execution_generation="generation", + kitchen_id="kitchen", + dispatch_id="", + binding_mode=PlanSetBindingMode.RECIPE, + ) + + result = await DefaultPlanSetMaterializer(_GitHub()).bind(request) + + assert result.reason is PlanSetRejectReason.COVERAGE_FAILED + assert result.coverage_gaps.strip() + if case != "unassigned": + for path in part_paths: + assert str(path) in result.coverage_gaps + for fragment in expected_gaps: + assert fragment in result.coverage_gaps + for fragment in expected_errors: + assert fragment in result.error + for fragment in expected_forms: + assert fragment in result.coverage_gaps + if expected_forms: + assert "Write a corrected plan part to a new file" in result.coverage_gaps + assert "keep the same requirements allocated" in result.coverage_gaps + + @pytest.mark.asyncio async def test_coverage_failure_writes_no_authority(tmp_path: Path) -> None: fixture = _FIXTURES / "missing_r2" diff --git a/tests/server/test_tools_plan_set.py b/tests/server/test_tools_plan_set.py index 827538d118..89365d8929 100644 --- a/tests/server/test_tools_plan_set.py +++ b/tests/server/test_tools_plan_set.py @@ -116,7 +116,9 @@ def now(_zone): assert repeated["plan_set_authority_path"] == initial["plan_set_authority_path"] -@pytest.mark.parametrize("case", ("empty", "outside", "fetch_failure", "no_issue")) +@pytest.mark.parametrize( + "case", ("empty", "outside", "fetch_failure", "no_issue", "coverage_failed") +) async def test_wrapper_classifies_inputs_and_failures( tmp_path: Path, tool_ctx, monkeypatch: pytest.MonkeyPatch, case: str ) -> None: @@ -139,14 +141,19 @@ async def test_wrapper_classifies_inputs_and_failures( assert result["reason"] == PlanSetRejectReason.ISSUE_FETCH_FAILED.value assert not list((root / "plan-set-authority").rglob("*.json")) else: + step_heading = "#### Step 1.1: P-1" if case == "coverage_failed" else "### Step 1.1: P-1" first.write_text( - "# Plan\n\n### Step 1.1: P-1\nImplement P-1.\n\n" + f"# Plan\n\n{step_heading}\nImplement P-1.\n\n" "## Issue Requirement Allocation\n" "| Requirement ID | Allocation | Implementation Step |\n" "|---|---|---|\n| P-1 | owned | Step 1.1 |\n", encoding="utf-8", ) result = json.loads(await bind_plan_set(plan_parts=str(first), cwd=str(tmp_path))) + if case == "coverage_failed": + assert result["reason"] == PlanSetRejectReason.COVERAGE_FAILED.value + assert result["coverage_gaps"].strip() + return assert result["success"] is True authority = json.loads(Path(result["plan_set_authority_path"]).read_text()) assert authority["inventory_mode"] == "no_issue" diff --git a/tests/workspace/test_project_local_overrides_resolution.py b/tests/workspace/test_project_local_overrides_resolution.py index 5b838b7724..d514a7a3fd 100644 --- a/tests/workspace/test_project_local_overrides_resolution.py +++ b/tests/workspace/test_project_local_overrides_resolution.py @@ -171,8 +171,8 @@ def test_matching_or_stricter_override_is_effective(tmp_path: Path) -> None: assert not resolver.scan_effective(tmp_path)[1] extended = bundled.replace( - "activate_deps:\n- write-recipe\n", - "activate_deps:\n- write-recipe\nrequires_resources:\n- arch-constraint-catalog\n", + "requires_resources:\n", + "requires_resources:\n- arch-constraint-catalog\n", 1, ) _write_make_plan_override(tmp_path, extended)