From ee90ee103d39f77fd0bc7d7ebe36d510b353cc86 Mon Sep 17 00:00:00 2001 From: Tom Softreck Date: Wed, 28 Oct 2026 15:25:38 +0100 Subject: [PATCH] fix(sync): read only selected GitHub issues before inbound updates --- planfile/sync/github.py | 37 +++ planfile/sync/operations.py | 29 +- project/TICKETS.md | 2 + project/ticket-253/README.md | 27 ++ project/ticket-253/intent.json | 80 +++++ tests/test_sync_selected_provider_reads.py | 325 +++++++++++++++++++++ 6 files changed, 491 insertions(+), 9 deletions(-) create mode 100644 project/ticket-253/README.md create mode 100644 project/ticket-253/intent.json create mode 100644 tests/test_sync_selected_provider_reads.py diff --git a/planfile/sync/github.py b/planfile/sync/github.py index 285eed5b..d4731b6c 100644 --- a/planfile/sync/github.py +++ b/planfile/sync/github.py @@ -13,6 +13,7 @@ from rich.console import Console from planfile.sync.base import BasePMBackend, TicketRef, TicketState +from planfile.sync.state import normalize_repository try: from github import Github @@ -669,6 +670,42 @@ def _issue_to_ticket_status(self, issue: Issue) -> TicketState: metadata=metadata, ) + def list_selected_tickets( + self, ticket_ids: set[str], labels: list[str] | None = None, + ) -> list[TicketState]: + """Read only resolved issue numbers, retaining exact provider bindings.""" + repository = normalize_repository( + getattr(self, "_verified_repository", None) or self.repo.full_name + ) + numbers = set() + for reference in ticket_ids: + number = str(reference).strip() + if "#" in number: + prefix, number = number.rsplit("#", 1) + if prefix and normalize_repository(prefix) != repository: + continue + if (number.isascii() and number.isdecimal() and int(number) > 0 + and str(int(number)) == number): + numbers.add(number) + tickets = [] + for number in sorted(numbers, key=int): + ticket = self.get_ticket(number) + key_repository, separator, key_number = str(ticket.key or "").rpartition("#") + url_repository, url_separator, url_number = str(ticket.url or "").removeprefix( + "https://github.com/" + ).rpartition("/issues/") + if (str(ticket.id) != number + or not separator or key_number != number + or normalize_repository(key_repository) != repository + or not str(ticket.url or "").startswith("https://github.com/") + or not url_separator or url_number != number + or normalize_repository(url_repository) != repository): + raise ValueError("sync_selected_provider_binding_mismatch") + if labels and not set(labels).issubset(ticket.labels): + continue + tickets.append(ticket) + return tickets + def _list_tickets( self, labels: list[str] | None = None, diff --git a/planfile/sync/operations.py b/planfile/sync/operations.py index 9b460331..3daafa66 100644 --- a/planfile/sync/operations.py +++ b/planfile/sync/operations.py @@ -440,11 +440,17 @@ def _fetch_external_tickets( backend, integration_name: str, labels: list[str] | None = None, + selected_remote_ids: set[str] | None = None, ) -> list | None: """Fetch tickets; preserve provider failures for caller retry and cooldown.""" try: kwargs = {"labels": labels} if labels else {} - external_tickets = backend.list_tickets(**kwargs) + selected_reader = getattr(backend, "list_selected_tickets", None) + if (integration_name == "github" and selected_remote_ids is not None + and callable(selected_reader)): + external_tickets = selected_reader(selected_remote_ids, **kwargs) + else: + external_tickets = backend.list_tickets(**kwargs) if external_tickets is None: console.print(f" [dim]ℹ️ No tickets found in {integration_name}[/dim]") return None @@ -634,20 +640,25 @@ def sync_from_external( return import_target = sections[target_id] lookup_sections = {target_id: import_target} - external_tickets = _fetch_external_tickets(backend, integration_name, remote_labels) - - if external_tickets is None: - return - if not external_tickets: - console.print(f" [dim]ℹ️ No tickets to import from {integration_name}[/dim]") - return - normalized_ticket_ids = ( {str(value).strip() for value in (ticket_ids or []) if str(value).strip()} or None ) + if normalized_ticket_ids: + for section in lookup_sections.values(): + for local_id, ticket in (section.get("tickets") or {}).items(): + if local_id in normalized_ticket_ids: + _validate_ticket_binding(ticket, integration_name, backend) selected_remote_ids = _resolve_selected_remote_ids( normalized_ticket_ids, lookup_sections, sync_state, integration_name ) + external_tickets = _fetch_external_tickets( + backend, integration_name, remote_labels, selected_remote_ids + ) + if external_tickets is None: + return + if not external_tickets: + console.print(f" [dim]ℹ️ No tickets to import from {integration_name}[/dim]") + return for ext_ticket in external_tickets: imported_count, updated_count = _process_external_ticket( diff --git a/project/TICKETS.md b/project/TICKETS.md index b09a18e6..b4025154 100644 --- a/project/TICKETS.md +++ b/project/TICKETS.md @@ -153,4 +153,6 @@ This file indexes governance tickets without taking ownership of | **ticket-248** | [`README.md`](./ticket-248/README.md) | - | - | - | - | - | | **ticket-249** | [`README.md`](./ticket-249/README.md) | - | - | - | - | - | | **ticket-250** | [`README.md`](./ticket-250/README.md) | - | - | - | - | - | +| **ticket-252** | [`README.md`](./ticket-252/README.md) | - | - | - | - | - | +| **ticket-253** | [`README.md`](./ticket-253/README.md) | - | - | - | - | - | diff --git a/project/ticket-253/README.md b/project/ticket-253/README.md new file mode 100644 index 00000000..920d0048 --- /dev/null +++ b/project/ticket-253/README.md @@ -0,0 +1,27 @@ +# Ticket 253: Exact scoped GitHub inbound reads + +- **ID**: ticket-253 +- **Owner**: agent:codex +- **Status**: IN_PROGRESS +- **Workflow state**: PUBLICATION +- **Created**: 2026-10-28 + +## Goal and scope + +SESSION_EXECUTION_AUTHORIZATION: continue user-requested Heyasa autonomy repair, tests and protected publication. Resume existing owner Planfile PLF-17304 / https://github.com/semcod/planfile/issues/256; no duplicate intake issue. + +Read only canonical mapped GitHub issues before enumeration in selected inbound sync. Preserve repository identity, labels, CAS, active execution guards and existing unscoped/provider compatibility. This slice retains the remaining total-deadline/transport requirements of256. + +## Acceptance criteria + +- [x] AC-01: Selected mapped intake avoids unrelated enumeration, rejects invalid packets and preserves unrelated/concurrent work. +- [x] AC-02: Current scoped regression/compatibility tests and native governance pass. +- [ ] AC-03: Required current-head/base verification and independent protected publication succeed. + +The original admission refusal is retained externally. Owner251 subsequently released its main lease at revision7/fence318; this session acquired a fresh main writer through the published controller. No ownership takeover or target/namespace substitution occurred. + +The historical proposal8d7f716ee0e043e3f54fe306231e9fcdc8caafd066005f2c4625ef7f07722e88 was requalified on base4adc9a2:667affected tests including60selected-provider cases passed. Current base8e2b186 changes managed metadata only; functional inputs planfile/**, tests/**, pyproject.toml and uv.lock are byte-identical. These external fixture results grant no CI/review/deployment authority; fresh native validation remains required. + +## Native validation + +Actual admitted worktree:667tests across26affected test files passed with Testless coverage contexts in191.44s; Ruff passes; native governance passes with0errors/0warnings. One existing SQLite ResourceWarning is retained in the test log. Testless generated693external advisory suggestions; none were imported or published as tasks. No runtime dependency, required check or protected policy changed. AC-03 remains pending independent verification/publication. diff --git a/project/ticket-253/intent.json b/project/ticket-253/intent.json new file mode 100644 index 00000000..7982cbde --- /dev/null +++ b/project/ticket-253/intent.json @@ -0,0 +1,80 @@ +{ + "schema": "new-project.intent/v3", + "ticket": "ticket-253", + "summary": "Read only mapped issues during scoped GitHub inbound sync", + "workstream": "application", + "classification": { + "kind": "BUG", + "priority": "P1", + "origin": "regression" + }, + "allowedPaths": [ + "project/ticket-253/**", + "TODO.md", + "project/TICKETS.md", + "planfile/sync/github.py", + "planfile/sync/operations.py", + "tests/test_sync_selected_provider_reads.py" + ], + "forbiddenPaths": [ + "project/ticket-*/user-*.md" + ], + "stacks": [], + "dependsOn": [], + "conflictsWith": [], + "integrationTicket": null, + "delivery": { + "outcome": "Read only canonical mapped GitHub issues for selected inbound sync while retaining full/unmapped provider compatibility, repository identity, labels, CAS and execution guards.", + "targetBranch": "main", + "acceptedBaseSha": "8e2b18637514adac380018f5ffb840a87f3e05a8", + "complexity": "S", + "estimatedMinutes": 45, + "budgets": { + "maxImplementationFiles": 3, + "maxAffectedComponents": 2, + "maxPublicInterfaceChanges": 1, + "maxRuntimeDependencies": 0 + }, + "nonGoals": [ + "No change to merge policy, lease ownership, provider credentials, full sync or runtime promotion.", + "This slice does not close every deadline/transport requirement in issue256." + ], + "architecture": { + "status": "accepted", + "decision": "Resume existing qualified unapplied256 proposal against current source; resolve canonical selections before fetch and use a provider capability for exact issue reads.", + "responsibilityChanges": false, + "components": [ + { + "name": "scoped-sync", + "paths": [ + "planfile/sync/operations.py", + "planfile/sync/github.py", + "tests/test_sync_selected_provider_reads.py" + ] + } + ], + "interfaceChanges": [ + "GitHubBackend.list_selected_tickets capability; existing CLI unchanged." + ], + "dataChanges": [], + "ui": { + "impact": "none", + "states": [], + "evidence": [] + }, + "rollback": "Protected revert of the source change; retain authoritative ticket records and remote terminal state." + }, + "runtimeDependencies": [], + "validation": [ + { + "criterion": "AC-01", + "commands": [ + "python -m pytest -q tests/test_sync_selected_provider_reads.py", + "python -m pytest -q tests/test_sync*.py", + "./project/governance-check.sh" + ], + "evidence": "Baseline desired contract fails; candidate exact/unmapped/sprint/identity/label/PR/failure/CAS/live-owner controls pass before full required current-head verification and independent publication." + } + ] + } +} diff --git a/tests/test_sync_selected_provider_reads.py b/tests/test_sync_selected_provider_reads.py new file mode 100644 index 00000000..8fc86df8 --- /dev/null +++ b/tests/test_sync_selected_provider_reads.py @@ -0,0 +1,325 @@ +"""Public SDK contracts: selection bounds provider I/O without waiving writes.""" + +from types import SimpleNamespace + +import pytest + +from planfile.core.store import PlanfileStore, Ticket, TicketUpdatedAtConflictError +from planfile.sync.github import GitHubBackend +from planfile.sync.operations import sync_from_external + + +@pytest.fixture(params=[False, True], ids=["yaml", "sharded"]) +def sample(tmp_path, request): + store = PlanfileStore(tmp_path) + store.init() + store.create_ticket( + Ticket( + id="PLF-001", + name="selected", + status="open", + sprint="backlog", + integration=["github"], + sync={"github": {"id": "553", "repository": "fixture/repo"}}, + ) + ) + store.create_ticket( + Ticket( + id="PLF-002", + name="unrelated", + status="open", + sprint="backlog", + integration=["github"], + sync={"github": {"id": "554", "repository": "fixture/repo"}}, + ) + ) + if request.param: + store.migrate_to_sharded_yaml() + calls = [] + + def issue(number): + return SimpleNamespace( + number=number, + title="remote " + str(number), + body="closed evidence", + state="closed", + state_reason="completed", + pull_request=None, + assignee=None, + labels=[SimpleNamespace(name="managed")], + updated_at=None, + html_url="https://github.com/fixture/repo/issues/" + str(number), + ) + + class Repo: + full_name = "fixture/repo" + callback = None + override = None + + def get_issues(self, **kwargs): + calls.append(("list", kwargs)) + raise AssertionError("Unrelated pages must not be read") + + def get_issue(self, number): + calls.append(("get", number)) + if self.callback: + self.callback(number) + return self.override or issue(number) + + backend = GitHubBackend.__new__(GitHubBackend) + backend.config = {"repo": "fixture/repo"} + backend.repo = Repo() + return store, backend, calls, issue + + +def pull(sample, selectors=None, **kwargs): + store, backend, *_ = sample + return sync_from_external( + backend, store, False, "github", ticket_ids=selectors or ["PLF-001"], **kwargs + ) + + +def unchanged(store): + return [store.get_ticket(id).model_dump(mode="json") for id in ["PLF-001", "PLF-002"]] + + +@pytest.mark.parametrize("selector", ["PLF-001", "553", "#553", "fixture/repo#553"]) +def test_exact_selected_sync_succeeds_when_unrelated_listing_is_unavailable(sample, selector): + store, backend, calls, _ = sample + before = store.get_ticket("PLF-001") + unrelated = unchanged(store)[1] + pull(sample, [selector]) + after = store.get_ticket("PLF-001") + assert ( + calls == [("get", 553)] and after.status == "done" and after.updated_at != before.updated_at + ) + assert ( + after.execution.state == "done" + and after.execution.assigned_to is None + and after.execution.lease_expires_at is None + ) + assert unchanged(store)[1] == unrelated + with pytest.raises(TicketUpdatedAtConflictError): + store.update_ticket(after.id, expected_updated_at=before.updated_at, description="stale") + + +def test_duplicate_selectors_read_once(sample): + pull(sample, ["PLF-001", "553", "#553", "fixture/repo#553"]) + assert sample[2] == [("get", 553)] + + +def test_multiple_reads_finish_before_any_ticket_write(sample): + store, backend, calls, _ = sample + before = unchanged(store) + + def callback(number): + assert unchanged(store) == before + if number == 554: + raise TimeoutError("selected provider failure") + + backend.repo.callback = callback + with pytest.raises(RuntimeError, match="sync_inbound_fetch_failed") as caught: + pull(sample, ["PLF-001", "PLF-002"]) + assert isinstance(caught.value.__cause__, TimeoutError) + assert calls == [("get", 553), ("get", 554)] and unchanged(store) == before + + +def test_selected_concurrent_mutation_rejects_stale_write(sample): + store, backend, *_ = sample + backend.repo.callback = lambda _: store.update_ticket( + "PLF-001", description="foreign concurrent owner" + ) + with pytest.raises(ValueError): + pull(sample) + assert ( + store.get_ticket("PLF-001").status == "open" + and store.get_ticket("PLF-001").description == "foreign concurrent owner" + ) + + +def test_live_execution_cannot_be_released_by_direct_read(sample): + store, *_ = sample + store.update_ticket( + "PLF-001", + status="in_progress", + execution={ + "state": "running", + "assigned_to": "foreign", + "lease_expires_at": "2099-01-01T00:00:00Z", + }, + ) + before = unchanged(store) + with pytest.raises(ValueError, match="execution_reconciliation"): + pull(sample) + assert unchanged(store) == before + + +def test_unrelated_concurrent_update_is_preserved(sample): + store, backend, *_ = sample + backend.repo.callback = lambda _: store.update_ticket( + "PLF-002", description="concurrent unrelated" + ) + pull(sample) + assert ( + store.get_ticket("PLF-001").status == "done" + and store.get_ticket("PLF-002").description == "concurrent unrelated" + ) + + +def test_direct_dry_run_never_changes_ticket(sample): + store, backend, calls, _ = sample + before = unchanged(store) + sync_from_external(backend, store, True, "github", ticket_ids=["PLF-001"]) + assert unchanged(store) == before and calls == [("get", 553)] + + +@pytest.mark.parametrize("labels", [["managed"], ["managed", "missing"]]) +def test_managed_label_filter_is_preserved(sample, labels): + store, *_ = sample + before = unchanged(store) + pull(sample, remote_labels=labels) + if len(labels) == 1: + assert store.get_ticket("PLF-001").status == "done" + else: + assert unchanged(store) == before + + +@pytest.mark.parametrize( + "selector", ["PLF-missing", "other/repo#553", "0553", "#0553", "0", "-1", "٥٥٣"] +) +def test_unresolved_selector_does_not_fall_back_to_listing(sample, selector): + store, _, calls, _ = sample + before = unchanged(store) + pull(sample, [selector]) + assert not calls and unchanged(store) == before + + +@pytest.mark.parametrize( + "field,value", + [ + ("number", 554), + ("html_url", "https://github.com/other/repo/issues/553"), + ("pull_request", {"url": "PR"}), + ], +) +def test_wrong_provider_packet_rejected_without_local_change(sample, field, value): + store, backend, _, issue = sample + before = unchanged(store) + packet = issue(553) + setattr(packet, field, value) + backend.repo.override = packet + with pytest.raises(RuntimeError, match="sync_inbound_fetch_failed"): + pull(sample) + assert unchanged(store) == before + + +def test_selected_cross_repository_mapping_rejected_before_issue_read(sample): + store, _, calls, _ = sample + t = store.get_ticket("PLF-001") + store.update_ticket(t.id, sync={"github": {"id": "553", "repository": "other/repo"}}) + before = unchanged(store) + with pytest.raises(ValueError, match="maps to"): + pull(sample) + assert not calls and unchanged(store) == before + + +def test_provider_repository_case_is_normalized(sample): + store, backend, _, issue = sample + backend.repo.full_name = "Fixture/Repo" + packet = issue(553) + packet.html_url = "https://github.com/Fixture/Repo/issues/553" + backend.repo.override = packet + pull(sample) + assert store.get_ticket("PLF-001").status == "done" + + +def test_unscoped_sync_retains_existing_listing_contract(sample): + store, backend, calls, issue = sample + + def list_issues(**kwargs): + calls.append(("list", kwargs)) + return [issue(553), issue(554)] + + backend.repo.get_issues = list_issues + sync_from_external(backend, store, False, "github") + assert calls == [("list", {"state": "all"})] and all( + store.get_ticket(id).status == "done" for id in ["PLF-001", "PLF-002"] + ) + + +def test_non_github_provider_keeps_existing_list_contract(sample): + store, _, calls, _ = sample + + class Backend: + config = {} + + def list_tickets(self, **kwargs): + calls.append(("legacy", kwargs)) + return [] + + def list_selected_tickets(self, *args, **kwargs): + raise AssertionError("GitHub capability is provider-specific") + + sync_from_external(Backend(), store, False, "fixture", ticket_ids=["PLF-001"]) + assert calls == [("legacy", {})] + + +def test_exact_issue_read_follows_fresh_numeric_repository_proof(sample): + store, backend, calls, _ = sample + + class Requester: + def requestJsonAndCheck(self, verb, path): + assert verb == "GET" + calls.append(("identity", path)) + return {}, {"id": 42, "full_name": "fixture/repo"} + + backend._identity_requester = Requester() + pull(sample) + assert calls == [ + ("identity", "/repos/fixture/repo"), + ("identity", "/repositories/42"), + ("get", 553), + ] + assert store.get_ticket("PLF-001").status == "done" + + +def test_numeric_identity_conflict_prevents_any_selected_issue_read(sample): + import yaml + + from planfile.sync.state import SyncState, SyncStateRepositoryMismatch + + store, backend, calls, _ = sample + before = unchanged(store) + state = SyncState(store.base_dir, "github", repository="fixture/repo") + state.save_sync({"PLF-001": "553"}) + raw = yaml.safe_load(state.state_file.read_text()) + raw["repository_id"] = "999" + state.state_file.write_text(yaml.safe_dump(raw)) + original = state.state_file.read_bytes() + + class Requester: + def requestJsonAndCheck(self, verb, path): + return {}, {"id": 42, "full_name": "fixture/repo"} + + backend._identity_requester = Requester() + with pytest.raises(SyncStateRepositoryMismatch): + pull(sample) + assert not calls and unchanged(store) == before and state.state_file.read_bytes() == original + + +def test_scoped_custom_sprint_updates_selected_ticket_in_place(sample): + store, _, calls, _ = sample + assert store.move_ticket("PLF-001", "custom-review") + pull(sample, sprint_ids=["custom-review"]) + assert ( + calls == [("get", 553)] + and store.get_ticket("PLF-001").sprint == "custom-review" + and store.get_ticket("PLF-001").status == "done" + ) + + +def test_unknown_sprint_does_not_read_selected_issue(sample): + store, _, calls, _ = sample + before = unchanged(store) + pull(sample, sprint_ids=["missing-sprint"]) + assert not calls and unchanged(store) == before