Skip to content

Commit fa4e7a9

Browse files
Trecekclaude
andcommitted
fix(fleet): canonical dual-key ingredient URL extraction (#4112)
The 7th occurrence of the label-orphaning bug class was caused by a singular/plural ingredient key mismatch: _api.py read only "issue_urls" (plural) but implementation/remediation recipes define "issue_url" (singular), silently producing _issue_urls_raw="" and defeating the DispatchRecord.issue_url fallback (deb5d55) and downstream label cleanup. Three-layer architectural immunity: 1. Canonical extraction function extract_issue_urls() in a new _issue_url_helpers.py module — single dual-key accessor (plural wins, singular fallback) used by _api.py:656. 2. AST guard (test_issue_url_extraction_guard.py): - BANS raw .get("issue_url") / .get("issue_urls") in fleet/ except in _issue_url_helpers.py (defines the canonical accessor) and state_types.py (deserializes DispatchRecord from its own JSON dict). - Asserts positive dual-key completeness in fleet_claim_guard.py — the stdlib-only hook cannot import the canonical helper and keeps its inline pattern; the test guarantees both key variants remain present. 3. Integration tests in test_dispatch_labels_cleaned.py dispatch with singular-key ingredients and no sidecar, asserting swap_labels fires via the issue_url fallback and DispatchRecord.issue_url is populated. Registry updates: - tests/arch/test_subpackage_isolation.py: fleet file-count ceiling 22 -> 23 - src/autoskillit/fleet/AGENTS.md: register _issue_url_helpers.py - tests/fleet/AGENTS.md: register test_issue_url_extraction.py - tests/arch/AGENTS.md: register test_issue_url_extraction_guard.py Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 8f5be90 commit fa4e7a9

9 files changed

Lines changed: 286 additions & 2 deletions

src/autoskillit/fleet/AGENTS.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ IL-2 fleet campaign layer — parallel issue dispatch, semaphore, sidecar, liven
1717
| `sidecar.py` | Per-issue JSONL sidecar — `IssueSidecarEntry`, append/read/`compute_remaining` helpers |
1818
| `_dispatch_reaper.py` | Stale dispatch process reaping — `reap_stale_dispatches()`, `reap_stale_dispatches_async()` |
1919
| `_label_cleanup.py` | Infrastructure-level label cleanup — `cleanup_orphaned_labels`, `sweep_stale_dispatch_labels`, `discover_campaign_state_files` |
20+
| `_issue_url_helpers.py` | Canonical issue-URL extraction — `extract_issue_urls()` dual-key accessor (plural `issue_urls` wins, singular `issue_url` fallback) |
2021
| `_liveness.py` | `is_dispatch_session_alive()` — boot_id + starttime_ticks liveness gate |
2122
| `_semaphore.py` | `FleetSemaphore` — configurable `asyncio.BoundedSemaphore` implementing `FleetLock` |
2223
| `_sidecar_rpc.py` | `run_python`-callable entry points: `write_sidecar_entry`, `get_remaining_issues` |

src/autoskillit/fleet/_api.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
)
2525
from autoskillit.fleet._capture import _extract_captures, _normalize_capture_spec
2626
from autoskillit.fleet._expressions import _CAMPAIGN_REF_RE, _interpolate_campaign_refs
27+
from autoskillit.fleet._issue_url_helpers import extract_issue_urls
2728
from autoskillit.fleet._outcome import _checkpoint_to_dict, classify_dispatch_outcome
2829
from autoskillit.fleet.result_parser import parse_l3_result_block
2930
from autoskillit.fleet.state import DispatchStatus
@@ -653,7 +654,7 @@ def _reject_with_state(error_code: FleetErrorCode, message: str) -> DispatchResu
653654
[f"%%L3_DONE::{pid[:8]}%%" for pid in prior_ids] if prior_ids else None
654655
)
655656

656-
_issue_urls_raw = effective_ingredients.get("issue_urls", "") if effective_ingredients else ""
657+
_issue_urls_raw = extract_issue_urls(effective_ingredients)
657658

658659
def _on_spawn(pid: int, ticks: int) -> None:
659660
from autoskillit.core import read_boot_id
Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
"""Canonical issue-URL extraction from recipe ingredients.
2+
3+
Single accessor for the dual-key ``issue_urls`` (plural, CSV) and ``issue_url``
4+
(singular) ingredient forms. Plural wins when both are present — it is the
5+
canonical recipe-side key for batch recipes (``bem-wrapper``, ``implement-findings``);
6+
singular is the fallback for single-issue recipes (``implementation``,
7+
``remediation``, ``research-*``).
8+
9+
Centralizing the lookup at a single function prevents the singular/plural
10+
mismatch that orphaned labels for the 7th time (issue #4112).
11+
"""
12+
13+
from __future__ import annotations
14+
15+
16+
def extract_issue_urls(ingredients: dict[str, str] | None) -> str:
17+
"""Return the issue-URL CSV from a recipe ingredients dict.
18+
19+
Tries ``issue_urls`` first (plural, batch recipes), then falls back to
20+
``issue_url`` (singular, single-issue recipes). Returns ``""`` if neither
21+
key is present, both values are empty strings, or the ingredients dict is
22+
``None``/empty.
23+
"""
24+
if not ingredients:
25+
return ""
26+
raw = ingredients.get("issue_urls", "")
27+
if not raw:
28+
raw = ingredients.get("issue_url", "")
29+
return raw

tests/arch/AGENTS.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,7 @@ AST enforcement, sub-package layer contracts, and architectural invariant tests.
110110
| `test_cmd_spec_resume_no_string_match.py` | AST guard: no `"--resume" in <expr>` patterns in production code — use CmdSpec.is_resume instead |
111111
| `test_subagent_filter_guard.py` | AST guard: all assistant-record NDJSON processing sites must use _is_parent_assistant_record or _is_parent_assistant predicate |
112112
| `test_swap_labels_guard.py` | AST guard: direct swap_labels calls in fleet/ must go through cleanup_orphaned_labels |
113+
| `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 |
113114
| `test_enqueue_ready_type_enforcement.py` | AST guard: mutation methods (_enqueue_direct, _enable_auto_merge_direct) must accept EnqueueReady, not str |
114115
| `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 |
115116
| `test_backend_annotation_context_awareness.py` | Context-aware pattern detection: distinguishes self-referential autoskillit: documentation from genuine cross-skill dependencies |
Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
"""AST guard: raw ``.get('issue_url')`` / ``.get('issue_urls')`` calls banned in ``fleet/``.
2+
3+
Issue #4112 defense-in-depth: the canonical extraction function lives in
4+
``_issue_url_helpers.py``, and any raw ``.get()`` call elsewhere in ``fleet/``
5+
(except ``_issue_url_helpers.py`` itself and ``state_types.py``, which
6+
deserializes ``DispatchRecord`` from its own JSON dict) would re-introduce the
7+
singular/plural key mismatch that orphaned labels for the 7th time.
8+
9+
Also enforces that ``fleet_claim_guard.py`` retains BOTH key variants in its
10+
inline dual-key lookup — the guard is a stdlib-only hook script and cannot
11+
import the canonical helper, so it keeps its inline pattern and this test
12+
guards that both ``issue_urls`` and ``issue_url`` keys remain present.
13+
"""
14+
15+
from __future__ import annotations
16+
17+
import ast
18+
from pathlib import Path
19+
20+
import pytest
21+
22+
pytestmark = [pytest.mark.layer("arch"), pytest.mark.small]
23+
24+
25+
SRC_ROOT = Path(__file__).resolve().parent.parent.parent / "src" / "autoskillit" / "fleet"
26+
HOOK_GUARD_PATH = (
27+
Path(__file__).resolve().parent.parent.parent
28+
/ "src"
29+
/ "autoskillit"
30+
/ "hooks"
31+
/ "guards"
32+
/ "fleet_claim_guard.py"
33+
)
34+
35+
# Files exempt from the raw-``get`` ban:
36+
# - ``_issue_url_helpers.py``: defines the canonical accessor.
37+
# - ``state_types.py``: deserializes ``DispatchRecord`` from its own JSON dict.
38+
EXEMPT_FILES: frozenset[str] = frozenset({"_issue_url_helpers.py", "state_types.py"})
39+
40+
BANNED_KEYS: frozenset[str] = frozenset({"issue_url", "issue_urls"})
41+
42+
43+
def _find_issue_url_get_calls(path: Path) -> list[tuple[int, str]]:
44+
"""Return ``(line, key)`` tuples for raw ``.get`` calls on issue-URL keys."""
45+
try:
46+
tree = ast.parse(path.read_text())
47+
except SyntaxError:
48+
return []
49+
results: list[tuple[int, str]] = []
50+
for node in ast.walk(tree):
51+
if not isinstance(node, ast.Call):
52+
continue
53+
func = node.func
54+
if not (isinstance(func, ast.Attribute) and func.attr == "get"):
55+
continue
56+
if not node.args:
57+
continue
58+
first_arg = node.args[0]
59+
if not isinstance(first_arg, ast.Constant):
60+
continue
61+
if first_arg.value in BANNED_KEYS:
62+
results.append((node.lineno, first_arg.value))
63+
return results
64+
65+
66+
def test_no_raw_issue_url_get_in_fleet() -> None:
67+
"""Raw ``.get('issue_url')`` / ``.get('issue_urls')`` are banned in fleet/.
68+
69+
All fleet code must route through ``extract_issue_urls()`` to guarantee the
70+
dual-key lookup is applied uniformly. Any direct ``.get()`` call would
71+
re-introduce the singular/plural mismatch.
72+
"""
73+
violations: list[str] = []
74+
for py_file in sorted(SRC_ROOT.glob("*.py")):
75+
if py_file.name in EXEMPT_FILES:
76+
continue
77+
for lineno, key in _find_issue_url_get_calls(py_file):
78+
violations.append(f"{py_file.name}:{lineno} (.get({key!r}))")
79+
80+
assert not violations, (
81+
"Raw .get('issue_url') / .get('issue_urls') calls in fleet/ must go "
82+
f"through extract_issue_urls() in _issue_url_helpers.py. Found in: {violations}"
83+
)
84+
85+
86+
def test_fleet_claim_guard_has_dual_key_lookup() -> None:
87+
"""``fleet_claim_guard.py`` must look up BOTH ``issue_urls`` AND ``issue_url``.
88+
89+
The hook is stdlib-only and cannot import ``extract_issue_urls()``, so it
90+
keeps its inline dual-key pattern. This test enforces that both key
91+
variants remain present — if a contributor removes either the plural or
92+
singular lookup, this test fails.
93+
"""
94+
assert HOOK_GUARD_PATH.exists(), f"Hook guard not found at {HOOK_GUARD_PATH}"
95+
found_keys = {key for _, key in _find_issue_url_get_calls(HOOK_GUARD_PATH)}
96+
97+
assert "issue_urls" in found_keys, (
98+
"fleet_claim_guard.py must look up the plural 'issue_urls' key."
99+
)
100+
assert "issue_url" in found_keys, (
101+
"fleet_claim_guard.py must look up the singular 'issue_url' key "
102+
"(dual-key ingredient access is required)."
103+
)

tests/arch/test_subpackage_isolation.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -878,7 +878,7 @@ def test_no_subpackage_exceeds_10_files() -> None:
878878
"cli": 21,
879879
"hooks": 14, # +recipe_confirmed_post_hook.py
880880
"pipeline": 12,
881-
"fleet": 22, # REQ-CNST-003-E9: _dispatch_reaper.py; +_sidecar_synthesis.py; +_reset.py
881+
"fleet": 23, # +_issue_url_helpers.py # noqa: E501
882882
"recipe/rules": 51, # +commit_guard_regression_route +rules_model +rules_gitignored_deliverable # noqa: E501
883883
"server/tools": 27, # +_preflight.py (dispatch-feasibility preflight)
884884
"hooks/guards": 32, # +fleet_claim_guard, +reset_resume_gate, +recipe_read_guard

tests/fleet/AGENTS.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ Fleet campaign dispatch, state persistence, and sidecar tests.
2323
| `test_dispatch_recipe_kind_gate.py` | Recipe kind dispatch gate — FOOD_TRUCK accepted, CAMPAIGN rejected |
2424
| `test_dispatch_crash_diagnostics.py` | Crash path diagnostic persistence and structured logging |
2525
| `test_dispatch_labels_cleaned.py` | Labels_cleaned field persistence on failure/success outcomes |
26+
| `test_issue_url_extraction.py` | Unit tests for `extract_issue_urls()` dual-key canonical accessor (singular/plural ingredient key resolution) |
2627
| `test_dispatch_identity_continuity.py` | Tests for dispatch_id identity continuity on resume — prior_dispatch_id threading through API layer |
2728
| `test_dispatch_state_handle.py` | Tests for DispatchStateHandle factory and dispatch state invariants — resume path state file creation and capture persistence (Group J) |
2829
| `test_dispatch_lifespan.py` | Group G (fleet part): lifespan_started surface + envelope propagation |

tests/fleet/test_dispatch_labels_cleaned.py

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -175,3 +175,98 @@ async def test_labels_cleaned_true_with_no_client_no_sidecar(
175175
record = _read_dispatch_record(tool_ctx)
176176
assert record["status"] == "failure"
177177
assert record["labels_cleaned"] is True
178+
179+
@pytest.mark.anyio
180+
async def test_singular_key_ingredient_triggers_fallback_cleanup(
181+
self, tool_ctx, monkeypatch: pytest.MonkeyPatch
182+
) -> None:
183+
"""Singular ``issue_url`` ingredient triggers cleanup via the ``issue_url`` fallback.
184+
185+
Without sidecar entries, ``cleanup_orphaned_labels`` falls back to the
186+
``issue_url`` kwarg. This test exercises that path with the singular
187+
``issue_url`` recipe ingredient (used by implementation/remediation).
188+
Regression guard for issue #4112.
189+
"""
190+
import dataclasses
191+
192+
from autoskillit.recipe.schema import RecipeIngredient
193+
from tests.fakes import _DEFAULT_SKILL_RESULT
194+
195+
issue_url = "https://github.com/owner/repo/issues/42"
196+
_setup_dispatch(
197+
tool_ctx,
198+
monkeypatch,
199+
ingredients={"issue_url": RecipeIngredient(description="Issue URL")},
200+
)
201+
swap_labels_mock = AsyncMock(return_value={"success": True})
202+
github_client = AsyncMock()
203+
github_client.swap_labels = swap_labels_mock
204+
tool_ctx.github_client = github_client
205+
206+
failure_result = dataclasses.replace(
207+
_DEFAULT_SKILL_RESULT,
208+
success=False,
209+
result='{"success": false, "reason": "context_exhaustion"}',
210+
subtype="success",
211+
is_error=False,
212+
exit_code=0,
213+
)
214+
# No sidecar is written — exercises the issue_url fallback path.
215+
tool_ctx.executor = InMemoryHeadlessExecutor(default_result=failure_result)
216+
monkeypatch.setattr(
217+
"autoskillit.fleet._api.parse_l3_result_block",
218+
lambda **_: _make_no_sentinel(),
219+
)
220+
221+
await _run(tool_ctx, ingredients={"issue_url": issue_url})
222+
223+
# swap_labels should have been called via the issue_url fallback path.
224+
swap_labels_mock.assert_awaited_once()
225+
call_list = swap_labels_mock.call_args_list
226+
assert len(call_list) == 1
227+
owner_arg, repo_arg, number_arg = call_list[0].args
228+
assert owner_arg == "owner"
229+
assert repo_arg == "repo"
230+
assert number_arg == 42
231+
232+
@pytest.mark.anyio
233+
async def test_singular_key_ingredient_populates_dispatch_record_issue_url(
234+
self, tool_ctx, monkeypatch: pytest.MonkeyPatch
235+
) -> None:
236+
"""DispatchRecord.issue_url is populated from the singular ``issue_url`` ingredient.
237+
238+
Regression guard for issue #4112: the field was previously stored as
239+
``""`` for single-issue recipes because ``_api.py`` only read the
240+
plural ``issue_urls`` key.
241+
"""
242+
import dataclasses
243+
244+
from autoskillit.recipe.schema import RecipeIngredient
245+
from tests.fakes import _DEFAULT_SKILL_RESULT
246+
247+
issue_url = "https://github.com/owner/repo/issues/42"
248+
_setup_dispatch(
249+
tool_ctx,
250+
monkeypatch,
251+
ingredients={"issue_url": RecipeIngredient(description="Issue URL")},
252+
)
253+
tool_ctx.github_client = None # skip label cleanup
254+
tool_ctx.executor = InMemoryHeadlessExecutor(
255+
default_result=dataclasses.replace(
256+
_DEFAULT_SKILL_RESULT,
257+
success=True,
258+
result='{"success": true}',
259+
subtype="success",
260+
is_error=False,
261+
exit_code=0,
262+
)
263+
)
264+
monkeypatch.setattr(
265+
"autoskillit.fleet._api.parse_l3_result_block",
266+
lambda **_: _make_completed_clean(True),
267+
)
268+
269+
await _run(tool_ctx, ingredients={"issue_url": issue_url})
270+
271+
record = _read_dispatch_record(tool_ctx)
272+
assert record["issue_url"] == issue_url
Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
"""Unit tests for ``extract_issue_urls()`` dual-key canonical accessor.
2+
3+
Validates the single canonical function that resolves the singular/plural
4+
ingredient-key mismatch that orphaned labels for the 7th time (issue #4112).
5+
"""
6+
7+
from __future__ import annotations
8+
9+
import pytest
10+
11+
from autoskillit.fleet._issue_url_helpers import extract_issue_urls
12+
13+
pytestmark = [pytest.mark.layer("fleet"), pytest.mark.small, pytest.mark.feature("fleet")]
14+
15+
16+
def test_plural_key_only() -> None:
17+
"""Batch recipes use the plural ``issue_urls`` key."""
18+
assert extract_issue_urls({"issue_urls": "url1,url2"}) == "url1,url2"
19+
20+
21+
def test_singular_key_only() -> None:
22+
"""Single-issue recipes use the singular ``issue_url`` key."""
23+
assert extract_issue_urls({"issue_url": "url1"}) == "url1"
24+
25+
26+
def test_plural_wins_when_both_present() -> None:
27+
"""Plural takes precedence when both keys are populated."""
28+
assert extract_issue_urls({"issue_urls": "url1", "issue_url": "url2"}) == "url1"
29+
30+
31+
def test_empty_dict() -> None:
32+
"""Empty ingredients dict returns empty string."""
33+
assert extract_issue_urls({}) == ""
34+
35+
36+
def test_none_ingredients() -> None:
37+
"""``None`` ingredients returns empty string (defensive guard)."""
38+
assert extract_issue_urls(None) == ""
39+
40+
41+
def test_empty_plural_falls_through_to_singular() -> None:
42+
"""Empty plural value falls through to singular lookup."""
43+
assert extract_issue_urls({"issue_urls": "", "issue_url": "url1"}) == "url1"
44+
45+
46+
def test_unrelated_keys_ignored() -> None:
47+
"""Other ingredient keys do not affect extraction."""
48+
assert extract_issue_urls({"label": "in-progress", "issue_url": "url1"}) == "url1"
49+
50+
51+
def test_both_empty_returns_empty() -> None:
52+
"""When both keys are empty, return empty string."""
53+
assert extract_issue_urls({"issue_urls": "", "issue_url": ""}) == ""

0 commit comments

Comments
 (0)