diff --git a/pdd/agentic_sync.py b/pdd/agentic_sync.py index f1aee8f65..35029b9c6 100644 --- a/pdd/agentic_sync.py +++ b/pdd/agentic_sync.py @@ -21,12 +21,15 @@ from rich.console import Console from .agentic_change import _check_gh_cli, _escape_format_braces, _parse_issue_url, _run_gh_command +from .agentic_change_orchestrator import _extract_marker_paths from .agentic_common import build_agentic_task_instruction, run_agentic_task from .agentic_sync_runner import ( AsyncSyncRunner, _architecture_entry_aliases, _basename_from_architecture_filename, + _basename_from_architecture_filepath, _find_pdd_executable, + _target_basename_aliases, build_dep_graph_from_architecture_data, ) from .durable_sync_runner import DurableSyncRunner @@ -228,6 +231,395 @@ def _detect_modules_from_branch_diff(project_root: Path) -> List[str]: return [] +# Backticked inline-code spans (single-line, non-empty) in issue markdown. +_ISSUE_BACKTICK_TOKEN_RE = re.compile(r"`([^`\n]+)`") +# Prompt-file path tokens (FILES_MODIFIED lists, prose references). Captures +# full non-whitespace paths so supported route-segment characters like the +# brackets in ``app/users/[id]/page_TypeScriptReact.prompt`` survive +# (PR #1983 round 3, P1#2); surrounding markdown punctuation is trimmed +# afterwards by ``_prompt_path_tokens``. +_ISSUE_PROMPT_PATH_RE = re.compile(r"\S+\.prompt\b") +# Leading punctuation markdown wraps around a path (backticks, quotes, parens, +# emphasis, list/link leftovers). The ``\.prompt`` anchor already excludes +# trailing punctuation. ``[`` is handled separately: it is markdown link syntax +# only when no matching ``]`` follows (otherwise it starts a dynamic route +# segment such as ``[id]/page_...``). +_PROMPT_PATH_LEAD_TRIM = "`\"'(<*,;:!]}>" +# Opening/closing fence line: a run of 3+ backticks or tildes (CommonMark). +_FENCE_LINE_RE = re.compile(r"(`{3,}|~{3,})") + + +def _prompt_path_tokens(text: str) -> List[str]: + """Extract prompt-file path tokens from text, trimming markdown wrapping.""" + tokens: List[str] = [] + for raw in _ISSUE_PROMPT_PATH_RE.findall(text): + token = raw.lstrip(_PROMPT_PATH_LEAD_TRIM) + while token.startswith("[") and "]" not in token: + token = token[1:] + if token: + tokens.append(token) + return tokens + + +def _strip_fenced_code_blocks(text: str) -> str: + """Remove fenced code blocks (``` or ~~~) from markdown text. + + Fenced content is quoted logs/snippets, not a request (PR #1983 round 3, + P2). An unclosed fence conservatively drops the remainder of the text — + under-matching noise is safe, while over-matching inflates a write-mode + sync scope. + + Fences may be LONGER than three characters: a fence closes only on a run + of the SAME character at least as long as the opener (CommonMark; + PR #1983 round 4, P2#2), so a 4-backtick fence swallows inner ``` pairs + as quoted content instead of leaking the text between them. + """ + kept: List[str] = [] + fence_char: Optional[str] = None + fence_len = 0 + for line in text.splitlines(): + match = _FENCE_LINE_RE.match(line.lstrip()) + if fence_char is None: + if match: + fence_char = match.group(1)[0] + fence_len = len(match.group(1)) + else: + kept.append(line) + continue + if match and match.group(1)[0] == fence_char and len(match.group(1)) >= fence_len: + fence_char = None + fence_len = 0 + return "\n".join(kept) + + +def _architecture_identity_index( + architecture: Optional[List[Dict[str, Any]]], +) -> Tuple[List[str], set, Dict[str, List[str]]]: + """Index REAL per-entry module identities, duplicates preserved. + + ``_architecture_module_basenames`` dedupes by derived basename, which + collapses distinct modules that share a prompt FILENAME — the normal + Next.js shape: several ``page_TypeScriptReact.prompt`` entries differing + only by route dir / ``filepath``. Extraction and coverage must instead see + one identity per distinct entry so a shared tail is detectably ambiguous + (PR #1983 round 3, P1#1). + + Returns ``(identities, known_keys, tail_to_identities)``: + + * ``identities`` — one primary identity per distinct entry (filename + derivation with directories preserved, falling back to the + filepath-derived key); duplicates KEPT. + * ``known_keys`` — every exact-match alias (filename- and + filepath-derived, via ``_architecture_entry_aliases``). + * ``tail_to_identities`` — final path component -> per-entry identities; + a tail listing more than one entry is ambiguous. + + Runtime ``*_LLM.prompt`` entries are excluded entirely. STRING-IDENTICAL + entries are deliberately NOT deduped (PR #1983 round 4, P1#1): the + combined inventory carries no source-project metadata + (``load_combined_architecture_data`` concatenates nested architecture + files without qualification), so two identical entries may be distinct + modules in different projects — treating them as ambiguous is the only + safe deterministic choice. The cost is recall-only and confined to a + module literally double-listed with identical strings (bare-name + resolution and tail coverage are disabled for it; exact/path-qualified + requests still work). + """ + identities: List[str] = [] + known_keys: set = set() + tail_to_identities: Dict[str, List[str]] = {} + for entry in architecture or []: + if not isinstance(entry, dict): + continue + filename = str(entry.get("filename", "") or "") + if _is_runtime_llm_template(filename): + continue + filepath = entry.get("filepath") + filepath_str = filepath if isinstance(filepath, str) else "" + primary = _basename_from_architecture_filename(filename) + if not primary and filepath_str: + primary = _basename_from_architecture_filepath(filepath_str) + if not primary: + continue + identities.append(primary) + known_keys.update(_architecture_entry_aliases(entry)) + tail_to_identities.setdefault(primary.rsplit("/", 1)[-1], []).append(primary) + return identities, known_keys, tail_to_identities + + +def _module_key_from_prompt_path(path_token: str) -> Optional[str]: + """Convert a prompt-file path token to a path-qualified module key. + + Mirrors the key derivation in ``_detect_modules_from_branch_diff`` (#1675): + ``extensions/b/prompts/src/page_python.prompt`` yields + ``extensions/b/src/page`` — the owning-project prefix (path before + ``prompts/``) plus the sub-path under ``prompts/`` plus the + language-stripped basename. Reducing the token to just the filename would + lose the qualification and let a same-tail sibling mask the request + (PR #1983 review, P1b). A path without a ``prompts/`` component keeps only + its filename basename (there is no key convention to preserve for it). + Runtime ``*_LLM.prompt`` templates yield ``None``. + """ + if path_token.startswith("prompts/"): + prefix, relative = "", path_token[len("prompts/"):] + else: + marker = "/prompts/" + idx = path_token.find(marker) + if idx == -1: + prefix, relative = "", path_token.rsplit("/", 1)[-1] + else: + prefix = path_token[: idx + 1] # keep trailing slash + relative = path_token[idx + len(marker):] + rel_path = Path(relative) + basename = extract_module_from_include(rel_path.name) + if not basename: + stem = rel_path.name + if stem.endswith(".prompt"): + stem = stem[: -len(".prompt")] + if not stem or _is_runtime_llm_template(stem): + return None + basename = stem + if rel_path.parent != Path("."): + return f"{prefix}{rel_path.parent.as_posix()}/{basename}" + return f"{prefix}{basename}" + + +def _resolve_issue_module_token( + token: str, + known_keys: set, + tail_to_identities: Dict[str, List[str]], +) -> Optional[str]: + """Resolve one candidate token to a known module key, or ``None``. + + Tries the token verbatim, then with ``.prompt`` / language suffixes + stripped. Rules over the REAL identity index (duplicates preserved): + + * An exact alias match resolves — but a BARE candidate whose tail lists + more than one distinct entry has no deterministic referent (two Next.js + ``page`` modules answer to the same bare name) and is skipped; a + path-qualified alias stays exact. + * A non-exact candidate resolves through its final path component only + when that tail lists exactly ONE entry and the qualification is + consistent (one side is a path suffix of the other, or the candidate is + bare). + + Runtime ``*_LLM`` templates never resolve (#1396 boundary). + """ + if _is_runtime_llm_template(token): + return None + candidates = [token] + if token.endswith(".prompt"): + candidates.append(token[: -len(".prompt")]) + stripped = _strip_language_suffix_preserving_path(candidates[-1]) + if stripped: + candidates.append(stripped) + for cand in candidates: + tail_matches = tail_to_identities.get(cand.rsplit("/", 1)[-1], []) + if cand in known_keys: + if "/" in cand or len(tail_matches) == 1: + return cand + continue + if len(tail_matches) == 1 and ( + "/" not in cand + or tail_matches[0].endswith(f"/{cand}") + or cand.endswith(f"/{tail_matches[0]}") + ): + return tail_matches[0] + return None + + +def _bare_mention_identities( + prose_text: str, + identities: List[str], + tail_to_identities: Dict[str, List[str]], +) -> List[str]: + """Class-3 scan: bare word-boundary mentions of identifier-like names. + + Only tails containing an underscore or hyphen qualify (they cannot be + ordinary prose words) and only when the tail lists exactly one real entry. + The lookarounds exclude adjacent word chars, ``/``, ``.``, and ``-`` so a + standalone mention matches but longer identifiers (``ci_validation_extra``, + ``double-check-run``), path segments (``a/ci_validation/b``), and filenames + (``ci_validation.py`` — file mentions go through class 2) do not. + """ + found: List[str] = [] + for module in identities: + tail = module.rsplit("/", 1)[-1] + if module in found or len(tail_to_identities[tail]) != 1: + continue + if "_" not in tail and "-" not in tail: + continue + if re.search(rf"(? List[str]: + """Collect prompt paths from trusted FILES_MODIFIED marker payloads only. + + Issue comments are largely workflow noise (step logs, retries), so free + text in them must never widen a write-mode sync scope (PR #1983 round 3, + P2). The only trusted signal is a ``FILES_MODIFIED:`` marker payload — the + exact format the change orchestrator emits — parsed with the same + ``_extract_marker_paths`` walker that produces/consumes those markers. + Fenced code blocks are stripped FIRST so a marker quoted inside a log + fence is treated as a log, not a directive. Only ``*.prompt`` payload + entries are returned; code/doc paths are not module requests. + """ + paths: List[str] = [] + for comment in comments or []: + if not isinstance(comment, dict): + continue + comment_body = str(comment.get("body", "") or "") + payload = _extract_marker_paths( + "FILES_MODIFIED", _strip_fenced_code_blocks(comment_body) + ) + paths.extend(p for p in payload if p.endswith(".prompt")) + return paths + + +def _explicit_token_modules( + text: str, + comments: Optional[List[Dict[str, Any]]], + known_keys: set, + tail_to_identities: Dict[str, List[str]], +) -> List[str]: + """Resolve class-1 and class-2 tokens to module keys, in mention order. + + Class 1: backticked single-word tokens from the title/body, resolved via + ``_resolve_issue_module_token``. Class 2: prompt-file paths from the + title/body plus trusted FILES_MODIFIED marker payloads from the filtered + comments. A path-qualified derived key whose tail exists in the inventory + is accepted directly: the path itself is the disambiguation, and the key + form matches what ``_detect_modules_from_branch_diff`` schedules for the + same file (#1675). Bare derived keys must still resolve uniquely. + """ + modules: List[str] = [] + + def _push(module: Optional[str]) -> None: + if module and module not in modules: + modules.append(module) + + for raw in _ISSUE_BACKTICK_TOKEN_RE.findall(text): + token = raw.strip() + if token and not any(ch.isspace() for ch in token): + _push(_resolve_issue_module_token(token, known_keys, tail_to_identities)) + + for path_token in _prompt_path_tokens(text) + _trusted_comment_prompt_paths(comments): + module_key = _module_key_from_prompt_path(path_token) + if not module_key: + continue + if "/" in module_key and module_key.rsplit("/", 1)[-1] in tail_to_identities: + _push(module_key) + else: + _push( + _resolve_issue_module_token(module_key, known_keys, tail_to_identities) + ) + return modules + + +def _extract_issue_named_modules( + title: str, + body: str, + architecture: Optional[List[Dict[str, Any]]], + comments: Optional[List[Dict[str, Any]]] = None, +) -> List[str]: + """Deterministically extract known modules explicitly named in an issue. + + Issue #1980: the branch-diff fast path must not silently drop modules the + issue explicitly requests. This helper matches the issue title/body — plus + trusted marker payloads from the FILTERED comments (FILES_MODIFIED markers + arrive as comments, PR #1983 review P1a) — against the REAL module + identity index (runtime ``*_LLM`` templates excluded) using three + high-precision, zero-LLM token classes: + + 1. Backticked inline-code tokens (e.g. ```greeter```, + ```prompts/greeter_python.prompt```) that resolve to a known + basename after optional ``.prompt``/language-suffix stripping. + 2. Prompt-file path tokens anywhere in the text (FILES_MODIFIED-style + lists such as ``prompts/textutil_python.prompt``), converted to + path-qualified module keys. + 3. Bare word-boundary mentions, but ONLY for basenames whose final + component contains an underscore or hyphen: identifier-like names such + as ``ci_validation`` or ``check-run`` cannot be ordinary prose words, + while a single-word module named e.g. ``python`` must not be pulled in + just because the word appears in prose (precision over recall — a + plain-prose mention of a single-word module is deliberately NOT + treated as an explicit request; use backticks or the prompt path). + + Scan surfaces (PR #1983 rounds 3-4, P2): ALL classes scan the title/body + with fenced code blocks stripped first (quoted logs/snippets are not + requests). Comments contribute ONLY trusted ``FILES_MODIFIED:`` marker + payloads (class 2), also fence-stripped first, never free text. + + Ambiguous tails (listing more than one real entry) never resolve from + bare names and a path-qualified prompt-path key is accepted as a + scheduler key directly — the same key form the branch-diff detector emits + (#1675). Returns keys in first-mention order; empty when there is no + architecture inventory to match against. + """ + identities, known_keys, tail_to_identities = _architecture_identity_index( + architecture + ) + if not identities: + return [] + + # Fence-strip ONCE, before every class (PR #1983 round 4, P2#1): a fenced + # prior-run log in the body must not widen scope through backtick or + # prompt-path tokens any more than through bare-name matches. + text = _strip_fenced_code_blocks(f"{title or ''}\n{body or ''}") + named: List[str] = [] + for module in _explicit_token_modules( + text, comments, known_keys, tail_to_identities + ) + _bare_mention_identities(text, identities, tail_to_identities): + if module not in named: + named.append(module) + return named + + +def _issue_modules_missing_from_scope( + scope_modules: List[str], + issue_modules: List[str], + architecture: Optional[List[Dict[str, Any]]], +) -> List[str]: + """Return issue-named modules not covered by the diff-detected scope. + + Diff-detected keys may be path-qualified (``extensions/foo/src/worker_app``) + while issue-named keys resolve from architecture basenames, so coverage + accepts exactly two forms: + + * exact full-key membership in the scope; or + * matching final path component, but ONLY when that tail lists exactly one + REAL entry in the identity index (pre-dedup, PR #1983 round 3 — two + ``page_TypeScriptReact.prompt`` entries are two modules even though the + deduped basename inventory collapses them). A globally-unique tail can + only refer to that one module, so key-form differences between the diff + derivation (repo path, #1675) and the architecture basename are safely + bridged. With duplicate tails, tail matching is disabled entirely: a + diff touching one sibling must not mask an explicit request for the + other, which then requires exact-key coverage. No looser path-suffix + matching is attempted — it reintroduces the same masking through bare + or partially-qualified keys. + """ + def _tail(module: str) -> str: + return module.rsplit("/", 1)[-1] + + _, _, tail_to_identities = _architecture_identity_index(architecture) + + scope_full = set(scope_modules) + scope_tails = {_tail(m) for m in scope_modules} + + def _covered(module: str) -> bool: + if module in scope_full: + return True + tail = _tail(module) + return len(tail_to_identities.get(tail, [])) <= 1 and tail in scope_tails + + return [m for m in issue_modules if not _covered(m)] + + def _branch_diff_is_runtime_llm_only(project_root: Path) -> bool: """Return True iff the branch diff vs. ``main`` consists only of runtime ``*_LLM.prompt`` template changes (with at least one such change present). @@ -1889,6 +2281,45 @@ def _resolve_module_cwd_and_target( return project_root, basename +def _dep_graph_target_for_module(rel_target: str, architecture: Any) -> str: + """Map a scheduler key to a dep-graph-resolvable target (PR #1983 round 4). + + Scheduler keys derived from the prompt SUBPATH (``app/settings/page``) + qualify by the directory under ``prompts/``, while a flat-filename + architecture entry only exposes filename- and filepath-derived aliases + (``page``, ``src/app/settings/page``). ``build_dep_graph_from_architecture_data`` + locates entries by exact alias, so such a key would silently resolve to no + entry and lose its declared dependency edges — the exact #1980 dropped-edge + failure, now on the widened scope. + + When ``rel_target`` already alias-matches some entry it is returned + unchanged. Otherwise, if it path-suffix-matches the aliases of exactly ONE + entry (one side is a ``/``-suffix of the other), that entry's matched + qualified alias is returned so the builder finds the right entry — the + bare filename alias is never used, since it may be shared across sibling + entries. Ambiguous or unmatched targets are returned unchanged (existing + no-guess behavior). + """ + entries = [e for e in (extract_modules(architecture) or []) if isinstance(e, dict)] + if not entries or "/" not in rel_target: + return rel_target + target_aliases = _target_basename_aliases(rel_target) + matched_aliases: List[str] = [] + for entry in entries: + aliases = _architecture_entry_aliases(entry) + if aliases & target_aliases: + return rel_target + for alias in sorted(aliases): + if "/" in alias and ( + alias.endswith(f"/{rel_target}") or rel_target.endswith(f"/{alias}") + ): + matched_aliases.append(alias) + break + if len(matched_aliases) == 1: + return matched_aliases[0] + return rel_target + + def _build_targeted_dep_graph( architecture: Any, modules: List[str], @@ -1927,29 +2358,40 @@ def _build_targeted_dep_graph( for bn in modules: cwd_groups.setdefault(info[bn][0].resolve(), []).append(bn) - root_resolved = project_root.resolve() graph: Dict[str, List[str]] = {bn: [] for bn in modules} warnings: List[str] = [] for cwd_resolved, group in cwd_groups.items(): - if cwd_resolved == root_resolved: + if cwd_resolved == project_root.resolve(): group_arch: Any = architecture else: group_arch, _ = _load_architecture_json(Path(cwd_resolved)) if not group_arch: group_arch = architecture + # PR #1983 round 4 (P1#2): a scheduler key derived from the prompt + # SUBPATH (``app/settings/page`` — emitted by both the branch-diff + # detector and issue prompt-path widening) may not alias-match a + # flat-filename entry whose aliases are filename-/filepath-derived + # (``page``, ``src/app/settings/page``); the builder would then + # silently return an empty dependency list. Pre-resolve such targets + # to the uniquely suffix-matched entry's resolvable alias, and remap + # the graph back to the original scheduler keys afterwards. + effective = { + bn: _dep_graph_target_for_module(info[bn][1], group_arch) + for bn in group + } # Relative targets are unique within a single project. - target_to_key = {info[bn][1]: bn for bn in group} + target_to_key = {effective[bn]: bn for bn in group} result = build_dep_graph_from_architecture_data( group_arch, - [info[bn][1] for bn in group], + [effective[bn] for bn in group], source_name=source_name, ) warnings.extend(result.warnings) for bn in group: graph[bn] = [ target_to_key.get(dep, dep) - for dep in result.graph.get(info[bn][1], []) + for dep in result.graph.get(effective[bn], []) ] return graph, warnings @@ -2745,6 +3187,26 @@ def run_agentic_sync( if not quiet: console.print(f"[green]Detected modules from branch diff: {branch_modules}[/green]") modules_to_sync = branch_modules + # Issue #1980: the branch diff reflects work already done, not the full + # request. Reconcile with the modules the issue explicitly names so the + # fast path never silently under-scopes: union any explicitly-named + # known module missing from the diff-detected set (still deterministic, + # zero LLM calls). When the diff already covers every issue-named + # module, behavior is unchanged. + issue_named_modules = _extract_issue_named_modules( + title, body, architecture, comments=filtered_comments + ) + issue_only_modules = _issue_modules_missing_from_scope( + branch_modules, issue_named_modules, architecture + ) + if issue_only_modules: + modules_to_sync = branch_modules + issue_only_modules + if not quiet: + console.print( + "[green]Issue explicitly names known modules missing from the " + f"branch diff; adding them to the sync scope: {issue_only_modules} " + f"(from branch diff: {branch_modules})[/green]" + ) elif _branch_diff_is_runtime_llm_only(project_root): # Hard boundary (Req 9 / issue #1396): the branch diff contains only # runtime *_LLM.prompt template changes. Those templates are consumed diff --git a/tests/test_agentic_sync.py b/tests/test_agentic_sync.py index df6660fe8..4436f3c0e 100644 --- a/tests/test_agentic_sync.py +++ b/tests/test_agentic_sync.py @@ -5692,3 +5692,887 @@ def test_hard_fail_posts_error_comment_when_github_state( # The posted comment carries the real oversize reason. posted_msg = mock_post_error.call_args.args[3] assert "input_too_large" in posted_msg + + +class TestBranchDiffIssueScopeReconciliation: + """Issue #1980: the branch-diff fast path must reconcile its scope with the + modules the issue explicitly names. + + Repro (epic PR #1868 E2E validation): the issue explicitly asks to sync + ``greeter`` and ``textutil``; the work-branch diff only touches ``greeter``. + The fast path synced only ``greeter``, dropped the greeter -> textutil + dependency edge with a yellow warning, and reported Success. The chosen + semantics: deterministically extract explicitly-named known modules from the + issue title/body and UNION any that are missing into the diff-detected + scope — still zero LLM calls. + """ + + _ARCH = [ + {"filename": "textutil_python.prompt", "dependencies": []}, + {"filename": "greeter_python.prompt", "dependencies": ["textutil_python.prompt"]}, + ] + + @staticmethod + def _issue(body: str) -> Dict[str, Any]: + return {"title": "Sync the greeter feature", "body": body, "comments_url": ""} + + # ------------------------------------------------------------------ + # Helper-level unit tests (deterministic extraction) + # ------------------------------------------------------------------ + + def test_extracts_backticked_known_modules(self): + from pdd.agentic_sync import _extract_issue_named_modules + + named = _extract_issue_named_modules( + "Sync the greeter feature", + "Please sync `greeter` and `textutil` together.", + self._ARCH, + ) + assert named == ["greeter", "textutil"] + + def test_extracts_prompt_file_paths(self): + """FILES_MODIFIED-style prompt paths resolve to known basenames.""" + from pdd.agentic_sync import _extract_issue_named_modules + + named = _extract_issue_named_modules( + "Sync the greeter feature", + "FILES_MODIFIED:\n" + "- prompts/greeter_python.prompt\n" + "- prompts/textutil_python.prompt\n", + self._ARCH, + ) + assert named == ["greeter", "textutil"] + + def test_prose_word_matching_single_word_module_is_not_extracted(self): + """A module named like an ordinary prose word (e.g. ``python``) must not + be pulled into scope just because the word appears in the issue text.""" + from pdd.agentic_sync import _extract_issue_named_modules + + arch = self._ARCH + [{"filename": "python_python.prompt", "dependencies": []}] + named = _extract_issue_named_modules( + "Fix the greeting", + "The `greeter` module is written in python and should say hi.", + arch, + ) + assert named == ["greeter"] + + def test_underscored_module_name_in_prose_is_extracted(self): + """Multi-word (underscored) basenames cannot be ordinary prose words, so a + bare word-boundary mention counts as an explicit reference.""" + from pdd.agentic_sync import _extract_issue_named_modules + + arch = [{"filename": "ci_validation_python.prompt", "dependencies": []}] + named = _extract_issue_named_modules( + "CI fails", "Update ci_validation to handle the new matrix.", arch + ) + assert named == ["ci_validation"] + + def test_underscored_name_inside_longer_token_is_not_extracted(self): + from pdd.agentic_sync import _extract_issue_named_modules + + arch = [{"filename": "ci_validation_python.prompt", "dependencies": []}] + named = _extract_issue_named_modules( + "CI fails", "See ci_validation_extra and prompts/ci_validation_python_old.py.", arch + ) + assert named == [] + + def test_runtime_llm_templates_never_extracted(self): + """#1396 boundary: runtime ``*_LLM.prompt`` mentions add nothing.""" + from pdd.agentic_sync import _extract_issue_named_modules + + arch = self._ARCH + [ + {"filename": "agentic_sync_identify_modules_LLM.prompt", "dependencies": []} + ] + named = _extract_issue_named_modules( + "Tweak templates", + "Edit `agentic_sync_identify_modules_LLM` and " + "prompts/agentic_sync_identify_modules_LLM.prompt.", + arch, + ) + assert named == [] + + def test_no_architecture_extracts_nothing(self): + from pdd.agentic_sync import _extract_issue_named_modules + + assert _extract_issue_named_modules("t", "sync `greeter`", None) == [] + assert _extract_issue_named_modules("t", "sync `greeter`", []) == [] + + # ------------------------------------------------------------------ + # run_agentic_sync integration tests (the actual issue shape) + # ------------------------------------------------------------------ + + @patch("pdd.agentic_sync.AsyncSyncRunner") + @patch("pdd.agentic_sync._run_dry_run_validation") + @patch( + "pdd.agentic_sync.build_dep_graph_from_architecture_data", + return_value=DepGraphFromArchitectureResult( + {"greeter": ["textutil"], "textutil": []}, [] + ), + ) + @patch("pdd.agentic_sync._detect_modules_from_branch_diff", return_value=["greeter"]) + @patch("pdd.agentic_sync.run_agentic_task") + @patch("pdd.agentic_sync._load_architecture_json") + @patch("pdd.agentic_sync._run_gh_command") + @patch("pdd.agentic_sync._check_gh_cli", return_value=True) + def test_union_issue_named_modules_missing_from_diff( + self, + mock_gh_cli, + mock_gh_cmd, + mock_load_arch, + mock_agentic_task, + mock_branch_diff, + mock_build_graph, + mock_dry_run, + mock_runner_cls, + ): + """Issue names ``greeter`` and ``textutil``; diff touches only greeter → + the final scope must include BOTH, with zero LLM calls.""" + mock_gh_cmd.return_value = ( + True, + json.dumps(self._issue("Please sync `greeter` and `textutil` together.")), + ) + mock_load_arch.return_value = (list(self._ARCH), Path("/tmp/architecture.json")) + mock_dry_run.return_value = ( + True, + {"greeter": Path("/tmp"), "textutil": Path("/tmp")}, + {"greeter": "greeter", "textutil": "textutil"}, + [], + 0.0, + ) + mock_runner = MagicMock() + mock_runner.run.return_value = (True, "All 2 modules synced successfully", 0.10) + mock_runner_cls.return_value = mock_runner + + success, msg, cost, model = run_agentic_sync( + "https://github.com/owner/repo/issues/1980", quiet=True + ) + + assert success is True + # The fast path stays free: no identify LLM call. + mock_agentic_task.assert_not_called() + # Both the diff-detected and the issue-named module reach dry-run. + mock_dry_run.assert_called_once() + dry_run_modules = mock_dry_run.call_args.kwargs.get( + "modules", + mock_dry_run.call_args.args[0] if mock_dry_run.call_args.args else None, + ) + assert sorted(dry_run_modules) == ["greeter", "textutil"] + # ...and the runner. + runner_kwargs = mock_runner_cls.call_args[1] + assert sorted(runner_kwargs["basenames"]) == ["greeter", "textutil"] + + @patch("pdd.agentic_sync.AsyncSyncRunner") + @patch("pdd.agentic_sync._run_dry_run_validation") + @patch( + "pdd.agentic_sync.build_dep_graph_from_architecture_data", + return_value=DepGraphFromArchitectureResult( + {"greeter": ["textutil"], "textutil": []}, [] + ), + ) + @patch( + "pdd.agentic_sync._detect_modules_from_branch_diff", + return_value=["greeter", "textutil"], + ) + @patch("pdd.agentic_sync.run_agentic_task") + @patch("pdd.agentic_sync._load_architecture_json") + @patch("pdd.agentic_sync._run_gh_command") + @patch("pdd.agentic_sync._check_gh_cli", return_value=True) + def test_diff_superset_of_issue_names_is_unchanged( + self, + mock_gh_cli, + mock_gh_cmd, + mock_load_arch, + mock_agentic_task, + mock_branch_diff, + mock_build_graph, + mock_dry_run, + mock_runner_cls, + ): + """Diff scope already covers the issue-named module → behavior unchanged + (same scope, no LLM call).""" + mock_gh_cmd.return_value = ( + True, + json.dumps(self._issue("Only `greeter` needs an update.")), + ) + mock_load_arch.return_value = (list(self._ARCH), Path("/tmp/architecture.json")) + mock_dry_run.return_value = ( + True, + {"greeter": Path("/tmp"), "textutil": Path("/tmp")}, + {"greeter": "greeter", "textutil": "textutil"}, + [], + 0.0, + ) + mock_runner = MagicMock() + mock_runner.run.return_value = (True, "All 2 modules synced successfully", 0.10) + mock_runner_cls.return_value = mock_runner + + success, msg, cost, model = run_agentic_sync( + "https://github.com/owner/repo/issues/1980", quiet=True + ) + + assert success is True + mock_agentic_task.assert_not_called() + dry_run_modules = mock_dry_run.call_args.kwargs.get( + "modules", + mock_dry_run.call_args.args[0] if mock_dry_run.call_args.args else None, + ) + assert dry_run_modules == ["greeter", "textutil"] + + @patch("pdd.agentic_sync.AsyncSyncRunner") + @patch("pdd.agentic_sync._run_dry_run_validation") + @patch( + "pdd.agentic_sync.build_dep_graph_from_architecture_data", + return_value=DepGraphFromArchitectureResult( + {"greeter": ["textutil"], "textutil": [], "python": []}, [] + ), + ) + @patch("pdd.agentic_sync._detect_modules_from_branch_diff", return_value=["greeter"]) + @patch("pdd.agentic_sync.run_agentic_task") + @patch("pdd.agentic_sync._load_architecture_json") + @patch("pdd.agentic_sync._run_gh_command") + @patch("pdd.agentic_sync._check_gh_cli", return_value=True) + def test_prose_non_module_words_do_not_inflate_scope( + self, + mock_gh_cli, + mock_gh_cmd, + mock_load_arch, + mock_agentic_task, + mock_branch_diff, + mock_build_graph, + mock_dry_run, + mock_runner_cls, + ): + """Prose mentioning a word that happens to be a module name (``python``) + must not inflate the diff-detected scope.""" + mock_gh_cmd.return_value = ( + True, + json.dumps( + self._issue("The greeting is wrong; it is written in python and says bye.") + ), + ) + arch = list(self._ARCH) + [{"filename": "python_python.prompt", "dependencies": []}] + mock_load_arch.return_value = (arch, Path("/tmp/architecture.json")) + mock_dry_run.return_value = ( + True, {"greeter": Path("/tmp")}, {"greeter": "greeter"}, [], 0.0 + ) + mock_runner = MagicMock() + mock_runner.run.return_value = (True, "All 1 modules synced successfully", 0.10) + mock_runner_cls.return_value = mock_runner + + success, msg, cost, model = run_agentic_sync( + "https://github.com/owner/repo/issues/1980", quiet=True + ) + + assert success is True + mock_agentic_task.assert_not_called() + dry_run_modules = mock_dry_run.call_args.kwargs.get( + "modules", + mock_dry_run.call_args.args[0] if mock_dry_run.call_args.args else None, + ) + assert dry_run_modules == ["greeter"] + + # ------------------------------------------------------------------ + # Round-2 review regressions (PR #1983 Codex findings) + # ------------------------------------------------------------------ + + _DUP_TAIL_ARCH = [ + {"filename": "extensions/a/src/page_python.prompt", "dependencies": []}, + {"filename": "extensions/b/src/page_python.prompt", "dependencies": []}, + ] + + def test_extracts_modules_from_filtered_comments(self): + """P1a: FILES_MODIFIED markers arrive as issue comments; extraction must + cover the same filtered comment content the identify path uses.""" + from pdd.agentic_sync import _extract_issue_named_modules + + comments = [ + {"user": {"login": "app-bot"}, "body": ( + "FILES_MODIFIED:\n- prompts/textutil_python.prompt\n" + )}, + ] + named = _extract_issue_named_modules( + "Sync the greeter feature", + "The greeting is wrong.", + self._ARCH, + comments=comments, + ) + assert named == ["textutil"] + + def test_low_signal_style_comment_content_not_required(self): + """Comments param is optional: omitting it keeps title/body behavior.""" + from pdd.agentic_sync import _extract_issue_named_modules + + named = _extract_issue_named_modules( + "Sync", "Fix `greeter`.", self._ARCH + ) + assert named == ["greeter"] + + def test_prompt_path_tokens_preserve_path_qualification(self): + """P1b(i): a nested prompt path must resolve to its path-qualified module + key, not collapse to an (ambiguous) bare filename basename.""" + from pdd.agentic_sync import _extract_issue_named_modules + + named = _extract_issue_named_modules( + "Fix page", + "FILES_MODIFIED:\n- extensions/b/prompts/src/page_python.prompt\n", + self._DUP_TAIL_ARCH, + ) + assert named == ["extensions/b/src/page"] + + def test_backticked_path_qualified_key_resolves_exactly(self): + from pdd.agentic_sync import _extract_issue_named_modules + + named = _extract_issue_named_modules( + "Fix page", + "Please fix `extensions/b/src/page` only.", + self._DUP_TAIL_ARCH, + ) + assert named == ["extensions/b/src/page"] + + def test_duplicate_tail_does_not_mask_missing_issue_module(self): + """P1b(ii): a diff touching extensions/a's ``page`` must not mask an + explicit request for extensions/b's ``page``.""" + from pdd.agentic_sync import _issue_modules_missing_from_scope + + missing = _issue_modules_missing_from_scope( + ["extensions/a/src/page"], + ["extensions/b/src/page"], + self._DUP_TAIL_ARCH, + ) + assert missing == ["extensions/b/src/page"] + + def test_unambiguous_tail_still_covers_bare_issue_module(self): + """Tail-based coverage stays for globally-unambiguous tails so diff keys + that are more qualified than architecture basenames do not re-add.""" + from pdd.agentic_sync import _issue_modules_missing_from_scope + + missing = _issue_modules_missing_from_scope( + ["extensions/proj/src/greeter"], + ["greeter"], + self._ARCH, + ) + assert missing == [] + + def test_exact_scope_key_always_covers(self): + from pdd.agentic_sync import _issue_modules_missing_from_scope + + missing = _issue_modules_missing_from_scope( + ["extensions/b/src/page"], + ["extensions/b/src/page"], + self._DUP_TAIL_ARCH, + ) + assert missing == [] + + def test_hyphenated_module_name_in_prose_is_extracted(self): + """P2: hyphenated identifier-like basenames (e.g. ``check-run``) cannot + be ordinary prose words either; a bare word-boundary mention counts.""" + from pdd.agentic_sync import _extract_issue_named_modules + + arch = [{"filename": "check-run_python.prompt", "dependencies": []}] + named = _extract_issue_named_modules( + "CI checks", "Update check-run so the checks pass.", arch + ) + assert named == ["check-run"] + + def test_hyphenated_name_inside_longer_token_is_not_extracted(self): + from pdd.agentic_sync import _extract_issue_named_modules + + arch = [{"filename": "check-run_python.prompt", "dependencies": []}] + named = _extract_issue_named_modules( + "CI checks", "See double-check-run and check-run-extra.", arch + ) + assert named == [] + + @patch("pdd.agentic_sync.AsyncSyncRunner") + @patch("pdd.agentic_sync._run_dry_run_validation") + @patch( + "pdd.agentic_sync.build_dep_graph_from_architecture_data", + return_value=DepGraphFromArchitectureResult( + {"greeter": ["textutil"], "textutil": []}, [] + ), + ) + @patch("pdd.agentic_sync._detect_modules_from_branch_diff", return_value=["greeter"]) + @patch("pdd.agentic_sync.run_agentic_task") + @patch("pdd.agentic_sync._load_architecture_json") + @patch("pdd.agentic_sync._run_gh_command") + @patch("pdd.agentic_sync._check_gh_cli", return_value=True) + def test_union_from_comment_only_files_modified( + self, + mock_gh_cli, + mock_gh_cmd, + mock_load_arch, + mock_agentic_task, + mock_branch_diff, + mock_build_graph, + mock_dry_run, + mock_runner_cls, + ): + """P1a integration: only an issue COMMENT names the second module via a + FILES_MODIFIED marker → the fast path must still union it.""" + issue = { + "title": "Sync the greeter feature", + "body": "The greeting is wrong.", + "comments_url": "https://api.github.com/repos/o/r/issues/1980/comments", + } + comments = [ + {"user": {"login": "app-bot"}, "body": ( + "FILES_MODIFIED:\n- prompts/textutil_python.prompt\n" + )}, + ] + mock_gh_cmd.side_effect = [ + (True, json.dumps(issue)), + (True, json.dumps(comments)), + ] + mock_load_arch.return_value = (list(self._ARCH), Path("/tmp/architecture.json")) + mock_dry_run.return_value = ( + True, + {"greeter": Path("/tmp"), "textutil": Path("/tmp")}, + {"greeter": "greeter", "textutil": "textutil"}, + [], + 0.0, + ) + mock_runner = MagicMock() + mock_runner.run.return_value = (True, "All 2 modules synced successfully", 0.10) + mock_runner_cls.return_value = mock_runner + + success, msg, cost, model = run_agentic_sync( + "https://github.com/owner/repo/issues/1980", quiet=True + ) + + assert success is True + mock_agentic_task.assert_not_called() + dry_run_modules = mock_dry_run.call_args.kwargs.get( + "modules", + mock_dry_run.call_args.args[0] if mock_dry_run.call_args.args else None, + ) + assert sorted(dry_run_modules) == ["greeter", "textutil"] + + @patch("pdd.agentic_sync.AsyncSyncRunner") + @patch("pdd.agentic_sync._run_dry_run_validation") + @patch( + "pdd.agentic_sync.build_dep_graph_from_architecture_data", + return_value=DepGraphFromArchitectureResult( + {"extensions/a/src/page": [], "extensions/b/src/page": []}, [] + ), + ) + @patch( + "pdd.agentic_sync._detect_modules_from_branch_diff", + return_value=["extensions/a/src/page"], + ) + @patch("pdd.agentic_sync.run_agentic_task") + @patch("pdd.agentic_sync._load_architecture_json") + @patch("pdd.agentic_sync._run_gh_command") + @patch("pdd.agentic_sync._check_gh_cli", return_value=True) + def test_union_path_qualified_duplicate_tail_not_masked( + self, + mock_gh_cli, + mock_gh_cmd, + mock_load_arch, + mock_agentic_task, + mock_branch_diff, + mock_build_graph, + mock_dry_run, + mock_runner_cls, + ): + """P1b integration: diff touches extensions/a's ``page``; issue names + extensions/b's ``page`` path-qualified → both must be in scope.""" + mock_gh_cmd.return_value = ( + True, + json.dumps({ + "title": "Fix page", + "body": "Please fix `extensions/b/src/page` as requested.", + "comments_url": "", + }), + ) + mock_load_arch.return_value = ( + list(self._DUP_TAIL_ARCH), Path("/tmp/architecture.json") + ) + mock_dry_run.return_value = ( + True, + {"extensions/a/src/page": Path("/tmp"), "extensions/b/src/page": Path("/tmp")}, + {"extensions/a/src/page": "page", "extensions/b/src/page": "page"}, + [], + 0.0, + ) + mock_runner = MagicMock() + mock_runner.run.return_value = (True, "All 2 modules synced successfully", 0.10) + mock_runner_cls.return_value = mock_runner + + success, msg, cost, model = run_agentic_sync( + "https://github.com/owner/repo/issues/1980", quiet=True + ) + + assert success is True + mock_agentic_task.assert_not_called() + dry_run_modules = mock_dry_run.call_args.kwargs.get( + "modules", + mock_dry_run.call_args.args[0] if mock_dry_run.call_args.args else None, + ) + assert sorted(dry_run_modules) == [ + "extensions/a/src/page", "extensions/b/src/page", + ] + + # ------------------------------------------------------------------ + # Round-3 review regressions (PR #1983 Codex round-2 findings) + # ------------------------------------------------------------------ + + # Real production shape (pdd_cloud frontend): multiple Next.js route + # modules share the same prompt FILENAME and differ only by route dir, + # including bracketed dynamic segments. + _NEXTJS_ARCH = [ + {"filename": "page_TypeScriptReact.prompt", + "filepath": "src/app/login/page.tsx", "dependencies": []}, + {"filename": "page_TypeScriptReact.prompt", + "filepath": "src/app/settings/page.tsx", "dependencies": []}, + {"filename": "page_TypeScriptReact.prompt", + "filepath": "src/app/users/[id]/page.tsx", "dependencies": []}, + ] + + def test_duplicate_filename_entries_do_not_collapse_to_one_identity(self): + """P1#1: two entries sharing a prompt filename are distinct modules; a + prompt-path request must resolve to its path-qualified scheduler key, + never to a bare collapsed basename.""" + from pdd.agentic_sync import _extract_issue_named_modules + + named = _extract_issue_named_modules( + "Fix settings page", + "FILES_MODIFIED:\n- prompts/app/settings/page_TypeScriptReact.prompt\n", + self._NEXTJS_ARCH, + ) + assert named == ["app/settings/page"] + + def test_bare_backtick_of_duplicated_tail_resolves_to_nothing(self): + """With duplicate real identities a bare ``page`` has no deterministic + referent — it must not resolve to the collapsed inventory name.""" + from pdd.agentic_sync import _extract_issue_named_modules + + named = _extract_issue_named_modules( + "Fix page", "Please fix `page` soon.", self._NEXTJS_ARCH + ) + assert named == [] + + def test_duplicate_filename_tail_does_not_cover_missing_module(self): + """P1#1 coverage: a diff touching the login page must not mask an + explicit request for the settings page just because the deduped + inventory collapsed both to one ``page`` identity.""" + from pdd.agentic_sync import _issue_modules_missing_from_scope + + missing = _issue_modules_missing_from_scope( + ["app/login/page"], + ["app/settings/page"], + self._NEXTJS_ARCH, + ) + assert missing == ["app/settings/page"] + + def test_bracketed_dynamic_route_prompt_path_is_tokenized(self): + """P1#2: ``[id]`` route segments must survive prompt-path tokenization + so the dynamic-route module resolves path-qualified.""" + from pdd.agentic_sync import _extract_issue_named_modules + + named = _extract_issue_named_modules( + "Fix user page", + "FILES_MODIFIED:\n- prompts/app/users/[id]/page_TypeScriptReact.prompt\n", + self._NEXTJS_ARCH, + ) + assert named == ["app/users/[id]/page"] + + def test_comment_prompt_path_outside_marker_is_ignored(self): + """P2(a): comments are scanned for trusted FILES_MODIFIED marker + payloads only — a prompt path in comment prose must not inflate scope.""" + from pdd.agentic_sync import _extract_issue_named_modules + + comments = [ + {"user": {"login": "app-bot"}, "body": ( + "Step 9/13 rebuilt prompts/textutil_python.prompt from scratch." + )}, + ] + named = _extract_issue_named_modules( + "Sync the greeter feature", + "Fix `greeter`.", + self._ARCH, + comments=comments, + ) + assert named == ["greeter"] + + def test_comment_marker_inside_log_fence_is_ignored(self): + """P2(a): a FILES_MODIFIED marker quoted inside a fenced log block in a + stale bot comment is a log, not a directive.""" + from pdd.agentic_sync import _extract_issue_named_modules + + comments = [ + {"user": {"login": "app-bot"}, "body": ( + "Step output was:\n" + "```\n" + "FILES_MODIFIED:\n" + "- prompts/textutil_python.prompt\n" + "```\n" + "Retrying later.\n" + )}, + ] + named = _extract_issue_named_modules( + "Sync the greeter feature", + "Fix `greeter`.", + self._ARCH, + comments=comments, + ) + assert named == ["greeter"] + + def test_comment_genuine_marker_line_still_extracts(self): + """P2(a) guard: an unfenced FILES_MODIFIED marker payload in a comment + keeps triggering reconciliation (round-2 P1a behavior).""" + from pdd.agentic_sync import _extract_issue_named_modules + + comments = [ + {"user": {"login": "app-bot"}, "body": ( + "FILES_MODIFIED:\n- prompts/textutil_python.prompt\n" + )}, + ] + named = _extract_issue_named_modules( + "Sync the greeter feature", + "The greeting is wrong.", + self._ARCH, + comments=comments, + ) + assert named == ["textutil"] + + def test_fenced_code_in_body_does_not_bare_name_match(self): + """P2(b): class-3 bare-name extraction must ignore fenced code blocks in + the issue body (logs/snippets are not requests).""" + from pdd.agentic_sync import _extract_issue_named_modules + + arch = [ + {"filename": "greeter_python.prompt", "dependencies": []}, + {"filename": "ci_validation_python.prompt", "dependencies": []}, + ] + named = _extract_issue_named_modules( + "Fix greeter", + "Fix `greeter`. The log said:\n" + "```\nERROR in ci_validation while running\n```\n", + arch, + ) + assert named == ["greeter"] + + def test_bare_name_outside_fence_still_matches(self): + from pdd.agentic_sync import _extract_issue_named_modules + + arch = [{"filename": "ci_validation_python.prompt", "dependencies": []}] + named = _extract_issue_named_modules( + "CI", "Please update ci_validation as well.\n```\nlog noise\n```\n", arch + ) + assert named == ["ci_validation"] + + @patch("pdd.agentic_sync.AsyncSyncRunner") + @patch("pdd.agentic_sync._run_dry_run_validation") + @patch( + "pdd.agentic_sync.build_dep_graph_from_architecture_data", + return_value=DepGraphFromArchitectureResult( + {"app/login/page": [], "app/settings/page": []}, [] + ), + ) + @patch( + "pdd.agentic_sync._detect_modules_from_branch_diff", + return_value=["app/login/page"], + ) + @patch("pdd.agentic_sync.run_agentic_task") + @patch("pdd.agentic_sync._load_architecture_json") + @patch("pdd.agentic_sync._run_gh_command") + @patch("pdd.agentic_sync._check_gh_cli", return_value=True) + def test_union_duplicate_filename_prompt_path_not_masked( + self, + mock_gh_cli, + mock_gh_cmd, + mock_load_arch, + mock_agentic_task, + mock_branch_diff, + mock_build_graph, + mock_dry_run, + mock_runner_cls, + ): + """P1#1 integration: diff touches the login page; the issue's + FILES_MODIFIED names the settings page prompt path → both pages must be + in scope, path-qualified, with zero LLM calls.""" + mock_gh_cmd.return_value = ( + True, + json.dumps({ + "title": "Fix settings page", + "body": ( + "FILES_MODIFIED:\n" + "- prompts/app/settings/page_TypeScriptReact.prompt\n" + ), + "comments_url": "", + }), + ) + mock_load_arch.return_value = ( + list(self._NEXTJS_ARCH), Path("/tmp/architecture.json") + ) + mock_dry_run.return_value = ( + True, + {"app/login/page": Path("/tmp"), "app/settings/page": Path("/tmp")}, + {"app/login/page": "page", "app/settings/page": "page"}, + [], + 0.0, + ) + mock_runner = MagicMock() + mock_runner.run.return_value = (True, "All 2 modules synced successfully", 0.10) + mock_runner_cls.return_value = mock_runner + + success, msg, cost, model = run_agentic_sync( + "https://github.com/owner/repo/issues/1980", quiet=True + ) + + assert success is True + mock_agentic_task.assert_not_called() + dry_run_modules = mock_dry_run.call_args.kwargs.get( + "modules", + mock_dry_run.call_args.args[0] if mock_dry_run.call_args.args else None, + ) + assert sorted(dry_run_modules) == ["app/login/page", "app/settings/page"] + + # ------------------------------------------------------------------ + # Round-4 review regressions (PR #1983 Codex round-3 findings) + # ------------------------------------------------------------------ + + # Two nested projects can legitimately declare STRING-IDENTICAL entries: + # load_combined_architecture_data concatenates architecture files without + # source-path qualification, so nothing distinguishes them in the combined + # list. They must be treated as ambiguous, never merged into one identity. + _CROSS_PROJECT_ARCH = [ + {"filename": "src/page_python.prompt", "filepath": "src/page.py", + "dependencies": []}, + {"filename": "src/page_python.prompt", "filepath": "src/page.py", + "dependencies": []}, + ] + + def test_string_identical_cross_project_entries_stay_ambiguous_for_coverage(self): + """P1#1: a diff touching project a's ``page`` must not cover an explicit + request for project b's ``page`` when the combined architecture carries + two string-identical (unqualifiable) entries.""" + from pdd.agentic_sync import _issue_modules_missing_from_scope + + missing = _issue_modules_missing_from_scope( + ["extensions/a/src/page"], + ["extensions/b/src/page"], + self._CROSS_PROJECT_ARCH, + ) + assert missing == ["extensions/b/src/page"] + + def test_string_identical_entries_bare_backtick_resolves_to_nothing(self): + """P1#1: with two indistinguishable entries a bare ``page`` has no + deterministic referent.""" + from pdd.agentic_sync import _extract_issue_named_modules + + named = _extract_issue_named_modules( + "Fix page", "Please fix `page` soon.", self._CROSS_PROJECT_ARCH + ) + assert named == [] + + def test_prompt_path_scheduler_key_keeps_declared_dependencies(self, tmp_path): + """P1#2: a prompt-subpath scheduler key (``app/settings/page``) must + still resolve its architecture entry's declared dependencies in the + targeted dep graph — flat-filename entries only expose filename-/ + filepath-derived aliases, which previously yielded a silent empty + dependency list (the exact #1980 dropped-edge failure).""" + import json as _json + from pdd.agentic_sync import _build_targeted_dep_graph + + arch = [ + {"filename": "page_TypeScriptReact.prompt", + "filepath": "src/app/login/page.tsx", "dependencies": []}, + {"filename": "page_TypeScriptReact.prompt", + "filepath": "src/app/settings/page.tsx", + "dependencies": ["textutil_python.prompt"]}, + {"filename": "textutil_python.prompt", + "filepath": "src/textutil.py", "dependencies": []}, + ] + (tmp_path / "architecture.json").write_text(_json.dumps(arch)) + + graph, warnings = _build_targeted_dep_graph( + arch, ["app/settings/page", "textutil"], tmp_path, "architecture.json" + ) + + assert graph["app/settings/page"] == ["textutil"] + assert graph["textutil"] == [] + + def test_ambiguous_suffix_target_left_unresolved_in_dep_graph(self, tmp_path): + """P1#2 guard: when a key suffix-matches more than one entry the target + stays unchanged (no guessing).""" + import json as _json + from pdd.agentic_sync import _build_targeted_dep_graph + + arch = [ + {"filename": "page_TypeScriptReact.prompt", + "filepath": "a/app/settings/page.tsx", + "dependencies": ["textutil_python.prompt"]}, + {"filename": "page_TypeScriptReact.prompt", + "filepath": "b/app/settings/page.tsx", + "dependencies": []}, + {"filename": "textutil_python.prompt", + "filepath": "src/textutil.py", "dependencies": []}, + ] + (tmp_path / "architecture.json").write_text(_json.dumps(arch)) + + graph, warnings = _build_targeted_dep_graph( + arch, ["app/settings/page", "textutil"], tmp_path, "architecture.json" + ) + + # Ambiguous: both entries' filepath aliases end with /app/settings/page. + assert graph["app/settings/page"] == [] + + def test_fenced_body_content_does_not_widen_scope_via_classes_1_and_2(self): + """P2#1: fenced prior-run logs in the issue BODY must not widen scope + through backtick or prompt-path tokens either.""" + from pdd.agentic_sync import _extract_issue_named_modules + + named = _extract_issue_named_modules( + "Sync the greeter feature", + "Fix `greeter`. Previous run log:\n" + "```\n" + "FILES_MODIFIED:\n" + "- prompts/textutil_python.prompt\n" + "then we touched `textutil` again\n" + "```\n", + self._ARCH, + ) + assert named == ["greeter"] + + def test_quad_backtick_fence_does_not_leak_inner_content(self): + """P2#2: a 4-backtick fence containing an inner triple-fenced snippet + must strip as ONE block (CommonMark: close needs same char, >= length).""" + from pdd.agentic_sync import _strip_fenced_code_blocks + + text = ( + "before\n" + "````\n" + "outer quoted\n" + "```\n" + "inner leaked?\n" + "```\n" + "still fenced\n" + "````\n" + "after\n" + ) + out = _strip_fenced_code_blocks(text) + assert "inner leaked?" not in out + assert "still fenced" not in out + assert "before" in out and "after" in out + + def test_quad_fence_comment_marker_does_not_extract(self): + """P2#2 end-to-end: a FILES_MODIFIED marker between inner triple fences + of a 4-backtick comment block must not widen scope.""" + from pdd.agentic_sync import _extract_issue_named_modules + + comments = [ + {"user": {"login": "app-bot"}, "body": ( + "````\n" + "```\n" + "FILES_MODIFIED:\n" + "- prompts/textutil_python.prompt\n" + "```\n" + "````\n" + )}, + ] + named = _extract_issue_named_modules( + "Sync the greeter feature", "Fix `greeter`.", self._ARCH, + comments=comments, + ) + assert named == ["greeter"] diff --git a/tests/test_agentic_sync_mocked_e2e.py b/tests/test_agentic_sync_mocked_e2e.py index 8277d8991..30fd1deab 100644 --- a/tests/test_agentic_sync_mocked_e2e.py +++ b/tests/test_agentic_sync_mocked_e2e.py @@ -340,11 +340,51 @@ def test_branch_diff_detection_skips_llm(harness): assert_no_billing(harness) +def test_branch_diff_scope_reconciled_with_issue_named_modules(harness): + """Issue #1980 repro (from the #1868 E2E validation): the fixture issue + explicitly names BOTH prompts (greeter and textutil) but the work branch + diff touches only greeter. The old fast path silently synced greeter alone, + dropping the greeter -> textutil dependency edge while reporting Success. + The reconciled fast path must scope both modules — still deterministically, + with zero provider calls.""" + prompt = harness.project / "prompts" / "greeter_python.prompt" + prompt.write_text(prompt.read_text() + "# friendlier greeting\n") + subprocess.run( + ["git", "commit", "-qam", "tweak greeter prompt"], + cwd=harness.project, env=harness.git_env, check=True, timeout=60, + ) + + result = run_sync(harness, "--dry-run", "--no-github-state") + + assert result.returncode == 0, result.stdout + result.stderr + out = result.stdout + assert "Detected modules from branch diff" in out + # The issue-named module missing from the diff was added, loudly. + assert "adding them to the sync scope" in out + assert "textutil" in out + + # BOTH modules reached real child dry-run validation subprocesses. + pdd_calls = read_log(harness, "pdd_calls.log") + assert ( + "--force --local sync greeter --dry-run --agentic --no-steer" + in pdd_calls + ) + assert ( + "--force --local sync textutil --dry-run --agentic --no-steer" + in pdd_calls + ) + + # Still the free path: no provider was ever invoked. + assert read_log(harness, "claude_calls.log") == "" + assert_no_billing(harness) + + def test_resume_skips_succeeded_modules(harness): """A state file marking every identified module as succeeded lets the execution phase complete without any provider or LLM call, and the state file is cleaned up on success.""" - # Branch-diff detection pins the module set to ['greeter'] (no LLM). + # Branch-diff detection finds ['greeter']; issue-scope reconciliation + # (#1980) adds the issue-named 'textutil' — still no LLM. prompt = harness.project / "prompts" / "greeter_python.prompt" prompt.write_text(prompt.read_text() + "# friendlier greeting\n") subprocess.run( @@ -352,28 +392,30 @@ def test_resume_skips_succeeded_modules(harness): cwd=harness.project, env=harness.git_env, check=True, timeout=60, ) + module_state = { + "status": "success", + "cost": 0.0, + "completed_phases": [], + "current_phase": None, + "start_time": None, + "end_time": None, + "error": None, + } state_dir = harness.project / ".pdd" state_dir.mkdir() (state_dir / "agentic_sync_state.json").write_text(json.dumps({ "issue_url": ISSUE_URL, "modules": { - "greeter": { - "status": "success", - "cost": 0.0, - "completed_phases": [], - "current_phase": None, - "start_time": None, - "end_time": None, - "error": None, - }, + "greeter": dict(module_state), + "textutil": dict(module_state), }, })) result = run_sync(harness, "--no-github-state") assert result.returncode == 0, result.stdout + result.stderr - assert "Resuming: skipping 1 already-succeeded module(s)" in result.stdout - assert "All 1 modules synced successfully" in result.stdout + assert "Resuming: skipping 2 already-succeeded module(s)" in result.stdout + assert "All 2 modules synced successfully" in result.stdout # State file removed after a fully successful run. assert not (state_dir / "agentic_sync_state.json").exists() assert_no_billing(harness)