diff --git a/src/autoskillit/fleet/AGENTS.md b/src/autoskillit/fleet/AGENTS.md index 1afcddf0b..c587c1117 100644 --- a/src/autoskillit/fleet/AGENTS.md +++ b/src/autoskillit/fleet/AGENTS.md @@ -17,6 +17,7 @@ IL-2 fleet campaign layer — parallel issue dispatch, semaphore, sidecar, liven | `sidecar.py` | Per-issue JSONL sidecar — `IssueSidecarEntry`, append/read/`compute_remaining` helpers | | `_dispatch_reaper.py` | Stale dispatch process reaping — `reap_stale_dispatches()`, `reap_stale_dispatches_async()` | | `_label_cleanup.py` | Infrastructure-level label cleanup — `cleanup_orphaned_labels`, `sweep_stale_dispatch_labels`, `discover_campaign_state_files` | +| `_issue_url_helpers.py` | Canonical issue-URL extraction — `extract_issue_urls()` dual-key accessor (plural `issue_urls` wins, singular `issue_url` fallback) | | `_liveness.py` | `is_dispatch_session_alive()` — boot_id + starttime_ticks liveness gate | | `_semaphore.py` | `FleetSemaphore` — configurable `asyncio.BoundedSemaphore` implementing `FleetLock` | | `_sidecar_rpc.py` | `run_python`-callable entry points: `write_sidecar_entry`, `get_remaining_issues` | diff --git a/src/autoskillit/fleet/_api.py b/src/autoskillit/fleet/_api.py index 73a2fcc1f..e7521643e 100644 --- a/src/autoskillit/fleet/_api.py +++ b/src/autoskillit/fleet/_api.py @@ -24,6 +24,7 @@ ) from autoskillit.fleet._capture import _extract_captures, _normalize_capture_spec from autoskillit.fleet._expressions import _CAMPAIGN_REF_RE, _interpolate_campaign_refs +from autoskillit.fleet._issue_url_helpers import extract_issue_urls from autoskillit.fleet._outcome import _checkpoint_to_dict, classify_dispatch_outcome from autoskillit.fleet.result_parser import parse_l3_result_block from autoskillit.fleet.state import DispatchStatus @@ -653,7 +654,7 @@ def _reject_with_state(error_code: FleetErrorCode, message: str) -> DispatchResu [f"%%L3_DONE::{pid[:8]}%%" for pid in prior_ids] if prior_ids else None ) - _issue_urls_raw = effective_ingredients.get("issue_urls", "") if effective_ingredients else "" + _issue_urls_raw = extract_issue_urls(effective_ingredients) def _on_spawn(pid: int, ticks: int) -> None: from autoskillit.core import read_boot_id diff --git a/src/autoskillit/fleet/_issue_url_helpers.py b/src/autoskillit/fleet/_issue_url_helpers.py new file mode 100644 index 000000000..c9727d374 --- /dev/null +++ b/src/autoskillit/fleet/_issue_url_helpers.py @@ -0,0 +1,29 @@ +"""Canonical issue-URL extraction from recipe ingredients. + +Single accessor for the dual-key ``issue_urls`` (plural, CSV) and ``issue_url`` +(singular) ingredient forms. Plural wins when both are present — it is the +canonical recipe-side key for batch recipes (``bem-wrapper``, ``implement-findings``); +singular is the fallback for single-issue recipes (``implementation``, +``remediation``, ``research-*``). + +Centralizing the lookup at a single function prevents the singular/plural +mismatch that orphaned labels for the 7th time (issue #4112). +""" + +from __future__ import annotations + + +def extract_issue_urls(ingredients: dict[str, str] | None) -> str: + """Return the issue-URL CSV from a recipe ingredients dict. + + Tries ``issue_urls`` first (plural, batch recipes), then falls back to + ``issue_url`` (singular, single-issue recipes). Returns ``""`` if neither + key is present, both values are empty strings, or the ingredients dict is + ``None``/empty. + """ + if not ingredients: + return "" + raw = ingredients.get("issue_urls", "") + if not raw: + raw = ingredients.get("issue_url", "") + return raw diff --git a/src/autoskillit/skills_extended/resolve-review/SKILL.md b/src/autoskillit/skills_extended/resolve-review/SKILL.md index 3edd379d8..94ba439a9 100644 --- a/src/autoskillit/skills_extended/resolve-review/SKILL.md +++ b/src/autoskillit/skills_extended/resolve-review/SKILL.md @@ -506,6 +506,7 @@ classified `REJECT` with `category: "arch_violation"`. | MCP env forward coverage | `test_mcp_env_forward_coverage.py` | `mcp_env_forward_vars` values missing from `CmdSpec.env` in any cmd-builder (skill, food-truck, headless, resume, interactive) | | Rule severity consistency | `test_rule_severity_consistency.py` | Direct `RuleFinding()` construction in `@semantic_rule`/`@block_rule` bodies — must use `make_finding()`/`make_block_finding()`; `_KNOWN_NON_CONFORMING_RULES` entries without `# tracking: #NNNN` comments | | No direct swap_labels in fleet | `test_swap_labels_guard.py` | Direct `swap_labels` calls in `fleet/` outside `_label_cleanup.py` — must route through `cleanup_orphaned_labels` | +| Issue URL extraction guard | `test_issue_url_extraction_guard.py` | Raw `.get("issue_url")` / `.get("issue_urls")` in `fleet/` outside `_issue_url_helpers.py` and `state_types.py` — must use `extract_issue_urls()`; `fleet_claim_guard.py` must retain both key variants | | Pipeline ordering | `test_pipeline_ordering.py` | Moving `run_semantic_rules` before `_prune_skipped_steps` in `load_and_validate` — semantic rules must run on post-prune recipe | When a reviewer suggestion would cause a change matching any row above, classify diff --git a/tests/_test_filter.py b/tests/_test_filter.py index c484dd3e1..6bd9c28dd 100644 --- a/tests/_test_filter.py +++ b/tests/_test_filter.py @@ -769,7 +769,7 @@ class ImportContext(enum.StrEnum): # Execution file-level entries: "execution/test_headless_path_validation.py", "execution/test_zero_write_detection.py", - # Fleet file-level entries (8 of N import autoskillit.recipe): + # Fleet file-level entries (9 of N import autoskillit.recipe): "fleet/test_fleet_e2e.py", "fleet/test_fleet_e2e_codex.py", "fleet/test_campaign_capture.py", @@ -780,6 +780,7 @@ class ImportContext(enum.StrEnum): "fleet/test_dispatch_state_handle.py", "fleet/test_research_campaign_dispatch.py", "fleet/test_dispatch_failure_semantics.py", + "fleet/test_dispatch_labels_cleaned.py", "fleet/test_gate_state_persistence.py", # Other file-level entries: "infra/test_pretty_output_recipe.py", diff --git a/tests/arch/AGENTS.md b/tests/arch/AGENTS.md index a87f342af..6507a8699 100644 --- a/tests/arch/AGENTS.md +++ b/tests/arch/AGENTS.md @@ -110,6 +110,7 @@ AST enforcement, sub-package layer contracts, and architectural invariant tests. | `test_cmd_spec_resume_no_string_match.py` | AST guard: no `"--resume" in ` patterns in production code — use CmdSpec.is_resume instead | | `test_subagent_filter_guard.py` | AST guard: all assistant-record NDJSON processing sites must use _is_parent_assistant_record or _is_parent_assistant predicate | | `test_swap_labels_guard.py` | AST guard: direct swap_labels calls in fleet/ must go through cleanup_orphaned_labels | +| `test_issue_url_extraction_guard.py` | AST guard: raw `.get('issue_url')` / `.get('issue_urls')` banned in fleet/; dual-key asserted in fleet_claim_guard.py | | `test_enqueue_ready_type_enforcement.py` | AST guard: mutation methods (_enqueue_direct, _enable_auto_merge_direct) must accept EnqueueReady, not str | | `test_origin_isolation_contract.py` | AST + shell lint guard: no hardcoded "origin" in git remote operations outside allowlist; shell scripts must try upstream before origin | | `test_backend_annotation_context_awareness.py` | Context-aware pattern detection: distinguishes self-referential autoskillit: documentation from genuine cross-skill dependencies | diff --git a/tests/arch/test_issue_url_extraction_guard.py b/tests/arch/test_issue_url_extraction_guard.py new file mode 100644 index 000000000..ab4901236 --- /dev/null +++ b/tests/arch/test_issue_url_extraction_guard.py @@ -0,0 +1,103 @@ +"""AST guard: raw ``.get('issue_url')`` / ``.get('issue_urls')`` calls banned in ``fleet/``. + +Issue #4112 defense-in-depth: the canonical extraction function lives in +``_issue_url_helpers.py``, and any raw ``.get()`` call elsewhere in ``fleet/`` +(except ``_issue_url_helpers.py`` itself and ``state_types.py``, which +deserializes ``DispatchRecord`` from its own JSON dict) would re-introduce the +singular/plural key mismatch that orphaned labels for the 7th time. + +Also enforces that ``fleet_claim_guard.py`` retains BOTH key variants in its +inline dual-key lookup — the guard is a stdlib-only hook script and cannot +import the canonical helper, so it keeps its inline pattern and this test +guards that both ``issue_urls`` and ``issue_url`` keys remain present. +""" + +from __future__ import annotations + +import ast +from pathlib import Path + +import pytest + +pytestmark = [pytest.mark.layer("arch"), pytest.mark.small] + + +SRC_ROOT = Path(__file__).resolve().parent.parent.parent / "src" / "autoskillit" / "fleet" +HOOK_GUARD_PATH = ( + Path(__file__).resolve().parent.parent.parent + / "src" + / "autoskillit" + / "hooks" + / "guards" + / "fleet_claim_guard.py" +) + +# Files exempt from the raw-``get`` ban: +# - ``_issue_url_helpers.py``: defines the canonical accessor. +# - ``state_types.py``: deserializes ``DispatchRecord`` from its own JSON dict. +EXEMPT_FILES: frozenset[str] = frozenset({"_issue_url_helpers.py", "state_types.py"}) + +BANNED_KEYS: frozenset[str] = frozenset({"issue_url", "issue_urls"}) + + +def _find_issue_url_get_calls(path: Path) -> list[tuple[int, str]]: + """Return ``(line, key)`` tuples for raw ``.get`` calls on issue-URL keys.""" + try: + tree = ast.parse(path.read_text()) + except SyntaxError: + return [] + results: list[tuple[int, str]] = [] + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + func = node.func + if not (isinstance(func, ast.Attribute) and func.attr == "get"): + continue + if not node.args: + continue + first_arg = node.args[0] + if not isinstance(first_arg, ast.Constant): + continue + if first_arg.value in BANNED_KEYS: + results.append((node.lineno, first_arg.value)) + return results + + +def test_no_raw_issue_url_get_in_fleet() -> None: + """Raw ``.get('issue_url')`` / ``.get('issue_urls')`` are banned in fleet/. + + All fleet code must route through ``extract_issue_urls()`` to guarantee the + dual-key lookup is applied uniformly. Any direct ``.get()`` call would + re-introduce the singular/plural mismatch. + """ + violations: list[str] = [] + for py_file in sorted(SRC_ROOT.glob("*.py")): + if py_file.name in EXEMPT_FILES: + continue + for lineno, key in _find_issue_url_get_calls(py_file): + violations.append(f"{py_file.name}:{lineno} (.get({key!r}))") + + assert not violations, ( + "Raw .get('issue_url') / .get('issue_urls') calls in fleet/ must go " + f"through extract_issue_urls() in _issue_url_helpers.py. Found in: {violations}" + ) + + +def test_fleet_claim_guard_has_dual_key_lookup() -> None: + """``fleet_claim_guard.py`` must look up BOTH ``issue_urls`` AND ``issue_url``. + + The hook is stdlib-only and cannot import ``extract_issue_urls()``, so it + keeps its inline dual-key pattern. This test enforces that both key + variants remain present — if a contributor removes either the plural or + singular lookup, this test fails. + """ + assert HOOK_GUARD_PATH.exists(), f"Hook guard not found at {HOOK_GUARD_PATH}" + found_keys = {key for _, key in _find_issue_url_get_calls(HOOK_GUARD_PATH)} + + assert "issue_urls" in found_keys, ( + "fleet_claim_guard.py must look up the plural 'issue_urls' key." + ) + assert "issue_url" in found_keys, ( + "fleet_claim_guard.py must look up the singular 'issue_url' key " + "(dual-key ingredient access is required)." + ) diff --git a/tests/arch/test_subpackage_isolation.py b/tests/arch/test_subpackage_isolation.py index f72838755..9ccc2733f 100644 --- a/tests/arch/test_subpackage_isolation.py +++ b/tests/arch/test_subpackage_isolation.py @@ -878,7 +878,7 @@ def test_no_subpackage_exceeds_10_files() -> None: "cli": 21, "hooks": 14, # +recipe_confirmed_post_hook.py "pipeline": 12, - "fleet": 22, # REQ-CNST-003-E9: _dispatch_reaper.py; +_sidecar_synthesis.py; +_reset.py + "fleet": 23, # +_issue_url_helpers.py # noqa: E501 "recipe/rules": 51, # +commit_guard_regression_route +rules_model +rules_gitignored_deliverable # noqa: E501 "server/tools": 27, # +_preflight.py (dispatch-feasibility preflight) "hooks/guards": 32, # +fleet_claim_guard, +reset_resume_gate, +recipe_read_guard diff --git a/tests/fleet/AGENTS.md b/tests/fleet/AGENTS.md index 63f38533c..6d991f1c5 100644 --- a/tests/fleet/AGENTS.md +++ b/tests/fleet/AGENTS.md @@ -23,6 +23,7 @@ Fleet campaign dispatch, state persistence, and sidecar tests. | `test_dispatch_recipe_kind_gate.py` | Recipe kind dispatch gate — FOOD_TRUCK accepted, CAMPAIGN rejected | | `test_dispatch_crash_diagnostics.py` | Crash path diagnostic persistence and structured logging | | `test_dispatch_labels_cleaned.py` | Labels_cleaned field persistence on failure/success outcomes | +| `test_issue_url_extraction.py` | Unit tests for `extract_issue_urls()` dual-key canonical accessor (singular/plural ingredient key resolution) | | `test_dispatch_identity_continuity.py` | Tests for dispatch_id identity continuity on resume — prior_dispatch_id threading through API layer | | `test_dispatch_state_handle.py` | Tests for DispatchStateHandle factory and dispatch state invariants — resume path state file creation and capture persistence (Group J) | | `test_dispatch_lifespan.py` | Group G (fleet part): lifespan_started surface + envelope propagation | diff --git a/tests/fleet/test_dispatch_labels_cleaned.py b/tests/fleet/test_dispatch_labels_cleaned.py index f641aa0ae..155095540 100644 --- a/tests/fleet/test_dispatch_labels_cleaned.py +++ b/tests/fleet/test_dispatch_labels_cleaned.py @@ -175,3 +175,98 @@ async def test_labels_cleaned_true_with_no_client_no_sidecar( record = _read_dispatch_record(tool_ctx) assert record["status"] == "failure" assert record["labels_cleaned"] is True + + @pytest.mark.anyio + async def test_singular_key_ingredient_triggers_fallback_cleanup( + self, tool_ctx, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Singular ``issue_url`` ingredient triggers cleanup via the ``issue_url`` fallback. + + Without sidecar entries, ``cleanup_orphaned_labels`` falls back to the + ``issue_url`` kwarg. This test exercises that path with the singular + ``issue_url`` recipe ingredient (used by implementation/remediation). + Regression guard for issue #4112. + """ + import dataclasses + + from autoskillit.recipe.schema import RecipeIngredient + from tests.fakes import _DEFAULT_SKILL_RESULT + + issue_url = "https://github.com/owner/repo/issues/42" + _setup_dispatch( + tool_ctx, + monkeypatch, + ingredients={"issue_url": RecipeIngredient(description="Issue URL")}, + ) + swap_labels_mock = AsyncMock(return_value={"success": True}) + github_client = AsyncMock() + github_client.swap_labels = swap_labels_mock + tool_ctx.github_client = github_client + + failure_result = dataclasses.replace( + _DEFAULT_SKILL_RESULT, + success=False, + result='{"success": false, "reason": "context_exhaustion"}', + subtype="success", + is_error=False, + exit_code=0, + ) + # No sidecar is written — exercises the issue_url fallback path. + tool_ctx.executor = InMemoryHeadlessExecutor(default_result=failure_result) + monkeypatch.setattr( + "autoskillit.fleet._api.parse_l3_result_block", + lambda **_: _make_no_sentinel(), + ) + + await _run(tool_ctx, ingredients={"issue_url": issue_url}) + + # swap_labels should have been called via the issue_url fallback path. + swap_labels_mock.assert_awaited_once() + call_list = swap_labels_mock.call_args_list + assert len(call_list) == 1 + owner_arg, repo_arg, number_arg = call_list[0].args + assert owner_arg == "owner" + assert repo_arg == "repo" + assert number_arg == 42 + + @pytest.mark.anyio + async def test_singular_key_ingredient_populates_dispatch_record_issue_url( + self, tool_ctx, monkeypatch: pytest.MonkeyPatch + ) -> None: + """DispatchRecord.issue_url is populated from the singular ``issue_url`` ingredient. + + Regression guard for issue #4112: the field was previously stored as + ``""`` for single-issue recipes because ``_api.py`` only read the + plural ``issue_urls`` key. + """ + import dataclasses + + from autoskillit.recipe.schema import RecipeIngredient + from tests.fakes import _DEFAULT_SKILL_RESULT + + issue_url = "https://github.com/owner/repo/issues/42" + _setup_dispatch( + tool_ctx, + monkeypatch, + ingredients={"issue_url": RecipeIngredient(description="Issue URL")}, + ) + tool_ctx.github_client = None # skip label cleanup + tool_ctx.executor = InMemoryHeadlessExecutor( + default_result=dataclasses.replace( + _DEFAULT_SKILL_RESULT, + success=True, + result='{"success": true}', + subtype="success", + is_error=False, + exit_code=0, + ) + ) + monkeypatch.setattr( + "autoskillit.fleet._api.parse_l3_result_block", + lambda **_: _make_completed_clean(True), + ) + + await _run(tool_ctx, ingredients={"issue_url": issue_url}) + + record = _read_dispatch_record(tool_ctx) + assert record["issue_url"] == issue_url diff --git a/tests/fleet/test_issue_url_extraction.py b/tests/fleet/test_issue_url_extraction.py new file mode 100644 index 000000000..0241f3aac --- /dev/null +++ b/tests/fleet/test_issue_url_extraction.py @@ -0,0 +1,53 @@ +"""Unit tests for ``extract_issue_urls()`` dual-key canonical accessor. + +Validates the single canonical function that resolves the singular/plural +ingredient-key mismatch that orphaned labels for the 7th time (issue #4112). +""" + +from __future__ import annotations + +import pytest + +from autoskillit.fleet._issue_url_helpers import extract_issue_urls + +pytestmark = [pytest.mark.layer("fleet"), pytest.mark.small, pytest.mark.feature("fleet")] + + +def test_plural_key_only() -> None: + """Batch recipes use the plural ``issue_urls`` key.""" + assert extract_issue_urls({"issue_urls": "url1,url2"}) == "url1,url2" + + +def test_singular_key_only() -> None: + """Single-issue recipes use the singular ``issue_url`` key.""" + assert extract_issue_urls({"issue_url": "url1"}) == "url1" + + +def test_plural_wins_when_both_present() -> None: + """Plural takes precedence when both keys are populated.""" + assert extract_issue_urls({"issue_urls": "url1", "issue_url": "url2"}) == "url1" + + +def test_empty_dict() -> None: + """Empty ingredients dict returns empty string.""" + assert extract_issue_urls({}) == "" + + +def test_none_ingredients() -> None: + """``None`` ingredients returns empty string (defensive guard).""" + assert extract_issue_urls(None) == "" + + +def test_empty_plural_falls_through_to_singular() -> None: + """Empty plural value falls through to singular lookup.""" + assert extract_issue_urls({"issue_urls": "", "issue_url": "url1"}) == "url1" + + +def test_unrelated_keys_ignored() -> None: + """Other ingredient keys do not affect extraction.""" + assert extract_issue_urls({"label": "in-progress", "issue_url": "url1"}) == "url1" + + +def test_both_empty_returns_empty() -> None: + """When both keys are empty, return empty string.""" + assert extract_issue_urls({"issue_urls": "", "issue_url": ""}) == ""