From 6f7b2c9234bb16ca5425279bdd966884a1591bf3 Mon Sep 17 00:00:00 2001 From: sohni-tagirisa <248822596+sohni-tagirisa@users.noreply.github.com> Date: Fri, 10 Jul 2026 13:42:20 -0700 Subject: [PATCH 1/8] test(sync): red tests for branch-diff fast-path under-scope vs issue-named modules (#1980) The #1868 E2E repro: issue explicitly names `greeter` and `textutil`, work-branch diff touches only greeter -> fast path syncs only greeter and reports Success. New tests assert the final scope must include both, with zero LLM calls, plus guards that a superset diff scope stays unchanged and prose words (e.g. a module named 'python') never inflate scope. Refs #1980 Co-Authored-By: Claude Fable 5 --- tests/test_agentic_sync.py | 278 +++++++++++++++++++++++++++++++++++++ 1 file changed, 278 insertions(+) diff --git a/tests/test_agentic_sync.py b/tests/test_agentic_sync.py index df6660fe8..a32d64d45 100644 --- a/tests/test_agentic_sync.py +++ b/tests/test_agentic_sync.py @@ -5692,3 +5692,281 @@ 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"] From 23af80d280e35748bc39c744e234c45566bdc5d8 Mon Sep 17 00:00:00 2001 From: sohni-tagirisa <248822596+sohni-tagirisa@users.noreply.github.com> Date: Fri, 10 Jul 2026 13:50:49 -0700 Subject: [PATCH 2/8] fix(sync): reconcile branch-diff fast-path scope with issue-named modules (#1980) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The branch-diff fast path in run_agentic_sync step 7 committed to modules_to_sync = branch_modules unconditionally, silently under-scoping relative to the modules the issue explicitly requests (repro from the #1868 E2E validation: issue names greeter AND textutil, diff touches only greeter -> only greeter synced, greeter->textutil edge dropped, Success reported). Fix: deterministically extract explicitly-named known modules from the issue title/body and UNION any that the diff missed into the scope — still zero LLM calls. Extraction matches only high-precision token classes against the architecture inventory: 1. backticked inline-code tokens resolving to a known basename (.prompt / language suffixes stripped), 2. prompt-file path tokens (FILES_MODIFIED-style lists), 3. bare word-boundary mentions, only for underscored basenames that cannot be ordinary prose words (a module named 'python' is never pulled in by prose; single-word modules require backticks or a prompt path). When the diff already covers every issue-named module, behavior is byte-for-byte unchanged. The added modules go through the existing normalization / invalid-basename / ambiguity pipeline like every other scope source. The #1396 runtime-LLM-only boundary and the #1883 PDD_CHANGED_MODULES authoritative-override arm are untouched. The mocked E2E suite gains the under-scope repro (test_branch_diff_scope_reconciled_with_issue_named_modules); the resume test's state fixture now marks both fixture modules succeeded since the fixture issue explicitly names both prompts. Fixes #1980 Co-Authored-By: Claude Fable 5 --- pdd/agentic_sync.py | 170 ++++++++++++++++++++++++++ tests/test_agentic_sync_mocked_e2e.py | 66 ++++++++-- 2 files changed, 224 insertions(+), 12 deletions(-) diff --git a/pdd/agentic_sync.py b/pdd/agentic_sync.py index f1aee8f65..8fe43c1b7 100644 --- a/pdd/agentic_sync.py +++ b/pdd/agentic_sync.py @@ -228,6 +228,158 @@ 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). +_ISSUE_PROMPT_PATH_RE = re.compile(r"[A-Za-z0-9_.\-][A-Za-z0-9_./\-]*\.prompt\b") + + +def _issue_candidate_tokens(text: str) -> List[str]: + """Collect high-precision explicit-module tokens from issue text. + + Two token classes (see ``_extract_issue_named_modules``): + backticked single-word inline-code spans, and prompt-file path tokens + (FILES_MODIFIED-style lists). For prompt paths only the filename is + consulted; ``extract_module_from_include`` strips the language suffix and + rejects runtime ``*_LLM.prompt`` templates (a non-language-suffixed + ``.prompt`` falls back to its bare stem). + """ + tokens: List[str] = [] + for raw in _ISSUE_BACKTICK_TOKEN_RE.findall(text): + token = raw.strip() + if token and not any(ch.isspace() for ch in token): + tokens.append(token) + for path_token in _ISSUE_PROMPT_PATH_RE.findall(text): + filename = path_token.rsplit("/", 1)[-1] + basename = extract_module_from_include(filename) + tokens.append(basename if basename else filename[: -len(".prompt")]) + return tokens + + +def _resolve_issue_module_token( + token: str, + known_set: set, + tail_to_modules: 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. A candidate that is not an exact known key may still resolve + through its final path component when that tail is UNAMBIGUOUS (exactly + one known module) 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: + if cand in known_set: + return cand + tail_matches = tail_to_modules.get(cand.rsplit("/", 1)[-1], []) + 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 _extract_issue_named_modules( + title: str, + body: str, + architecture: Optional[List[Dict[str, Any]]], +) -> 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 against + the KNOWN module inventory (architecture basenames, runtime ``*_LLM`` + templates already excluded by ``_architecture_module_basenames``) 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``). + 3. Bare word-boundary mentions, but ONLY for basenames whose final + component contains an underscore: multi-word names like + ``ci_validation`` 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 to request it). + + Ambiguous bare tails (mapping to more than one known module) are skipped; + downstream #1677 handling requires path-qualified names for those anyway. + Returns known-module keys in first-mention order; empty when there is no + architecture inventory to match against. + """ + known = _architecture_module_basenames(architecture or []) + if not known: + return [] + known_set = set(known) + # Map each final path component to its full module keys so a bare mention + # of "worker_app" can resolve "extensions/foo/src/worker_app". + tail_to_modules: Dict[str, List[str]] = {} + for module in known: + tail_to_modules.setdefault(module.rsplit("/", 1)[-1], []).append(module) + + text = f"{title or ''}\n{body or ''}" + named: List[str] = [] + seen: set[str] = set() + + # Classes 1 + 2: backticked tokens and prompt-file paths. + for token in _issue_candidate_tokens(text): + resolved = _resolve_issue_module_token(token, known_set, tail_to_modules) + if resolved and resolved not in seen: + named.append(resolved) + seen.add(resolved) + + # Class 3: bare word-boundary mentions of multi-word (underscored) names. + for module in known: + tail = module.rsplit("/", 1)[-1] + if module in seen or "_" not in tail or len(tail_to_modules[tail]) != 1: + continue + # No word chars, '/', '.', or '-' adjacent: matches a standalone + # mention but not longer identifiers (ci_validation_extra), path + # segments (a/ci_validation/b), or filenames (ci_validation.py is + # intentionally excluded — file mentions go through class 2). + if re.search(rf"(? 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 a module is + considered covered when either its full key or its final path component is + already present in the scope (by full key or tail). + """ + def _tail(module: str) -> str: + return module.rsplit("/", 1)[-1] + + scope_keys = set(scope_modules) | {_tail(m) for m in scope_modules} + return [ + m for m in issue_modules + if m not in scope_keys and _tail(m) not in scope_keys + ] + + 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). @@ -2745,6 +2897,24 @@ 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) + issue_only_modules = _issue_modules_missing_from_scope( + branch_modules, issue_named_modules + ) + 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_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) From 5862db26b5407fbd0616286408cf1d9418a263b7 Mon Sep 17 00:00:00 2001 From: sohni-tagirisa <248822596+sohni-tagirisa@users.noreply.github.com> Date: Fri, 10 Jul 2026 15:57:35 -0700 Subject: [PATCH 3/8] test(sync): red tests for round-2 review findings on issue-scope reconciliation (#1980) PR #1983 Codex review: - P1a: extraction must cover filtered issue comments (comment-only FILES_MODIFIED markers), not just title/body. - P1b: prompt-path tokens must keep path qualification, and tail-based scope coverage must only apply to globally-unambiguous tails so a diff touching extensions/a/src/page cannot mask an explicit request for extensions/b/src/page. - P2: bare hyphenated basenames (check-run) are valid inventory members and must prose-match like underscored ones. Refs #1980 Co-Authored-By: Claude Fable 5 --- tests/test_agentic_sync.py | 241 +++++++++++++++++++++++++++++++++++++ 1 file changed, 241 insertions(+) diff --git a/tests/test_agentic_sync.py b/tests/test_agentic_sync.py index a32d64d45..3595769d5 100644 --- a/tests/test_agentic_sync.py +++ b/tests/test_agentic_sync.py @@ -5970,3 +5970,244 @@ def test_prose_non_module_words_do_not_inflate_scope( 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", + ] From a297a5303d0dc2777c3c58093387fee9fbd621c7 Mon Sep 17 00:00:00 2001 From: sohni-tagirisa <248822596+sohni-tagirisa@users.noreply.github.com> Date: Fri, 10 Jul 2026 16:03:09 -0700 Subject: [PATCH 4/8] =?UTF-8?q?fix(sync):=20address=20round-2=20review=20?= =?UTF-8?q?=E2=80=94=20comment=20extraction,=20qualified=20keys,=20strict?= =?UTF-8?q?=20tail=20coverage=20(#1980)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #1983 Codex review fixes: P1a: _extract_issue_named_modules gains a comments parameter and the step-7 call site passes the already-filtered signal-bearing comments (post-_filter_low_signal_comments), so comment-only FILES_MODIFIED markers now trigger reconciliation. Only comment bodies are scanned. P1b: prompt-path tokens now convert to path-qualified module keys via _module_key_from_prompt_path (same derivation as _detect_modules_from_branch_diff, #1675) instead of collapsing to the filename; _issue_modules_missing_from_scope takes the architecture inventory and accepts tail-based coverage ONLY for globally-unambiguous tails — duplicate-tail siblings now require exact-key coverage, so a diff touching extensions/a/src/page can no longer mask an explicit request for extensions/b/src/page. No looser path-suffix matching is attempted (it reintroduces masking through bare keys). P2: the bare prose matcher gate extends from underscore-only to underscore-or-hyphen tails (check-run); verified hyphenated basenames survive _basename_from_architecture_filename, and the existing boundary lookarounds already exclude hyphen-adjacent longer identifiers. Refs #1980 Co-Authored-By: Claude Fable 5 --- pdd/agentic_sync.py | 152 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 118 insertions(+), 34 deletions(-) diff --git a/pdd/agentic_sync.py b/pdd/agentic_sync.py index 8fe43c1b7..de18aefe9 100644 --- a/pdd/agentic_sync.py +++ b/pdd/agentic_sync.py @@ -234,15 +234,51 @@ def _detect_modules_from_branch_diff(project_root: Path) -> List[str]: _ISSUE_PROMPT_PATH_RE = re.compile(r"[A-Za-z0-9_.\-][A-Za-z0-9_./\-]*\.prompt\b") +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 _issue_candidate_tokens(text: str) -> List[str]: """Collect high-precision explicit-module tokens from issue text. Two token classes (see ``_extract_issue_named_modules``): backticked single-word inline-code spans, and prompt-file path tokens - (FILES_MODIFIED-style lists). For prompt paths only the filename is - consulted; ``extract_module_from_include`` strips the language suffix and - rejects runtime ``*_LLM.prompt`` templates (a non-language-suffixed - ``.prompt`` falls back to its bare stem). + (FILES_MODIFIED-style lists). Prompt paths convert to path-qualified + module keys via ``_module_key_from_prompt_path``; runtime ``*_LLM.prompt`` + templates are rejected there and again in the downstream resolver. """ tokens: List[str] = [] for raw in _ISSUE_BACKTICK_TOKEN_RE.findall(text): @@ -250,9 +286,9 @@ def _issue_candidate_tokens(text: str) -> List[str]: if token and not any(ch.isspace() for ch in token): tokens.append(token) for path_token in _ISSUE_PROMPT_PATH_RE.findall(text): - filename = path_token.rsplit("/", 1)[-1] - basename = extract_module_from_include(filename) - tokens.append(basename if basename else filename[: -len(".prompt")]) + module_key = _module_key_from_prompt_path(path_token) + if module_key: + tokens.append(module_key) return tokens @@ -291,31 +327,52 @@ def _resolve_issue_module_token( return None +def _issue_scan_text( + title: str, + body: str, + comments: Optional[List[Dict[str, Any]]], +) -> str: + """Join the issue title, body, and comment BODIES for module scanning. + + Only comment bodies are included (not author logins or structural + markers), so a username can never resolve to a module. + """ + parts = [title or "", body or ""] + for comment in comments or []: + if isinstance(comment, dict): + parts.append(str(comment.get("body", "") or "")) + return "\n".join(parts) + + 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 against - the KNOWN module inventory (architecture basenames, runtime ``*_LLM`` - templates already excluded by ``_architecture_module_basenames``) using - three high-precision, zero-LLM token classes: + issue explicitly requests. This helper matches the issue title/body — plus + the FILTERED comment bodies (the same signal-bearing set the identify path + consumes; FILES_MODIFIED markers arrive as comments, PR #1983 review P1a) + — against the KNOWN module inventory (architecture basenames, runtime + ``*_LLM`` templates already excluded by ``_architecture_module_basenames``) + 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``). + 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: multi-word names like - ``ci_validation`` 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 to request it). + 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). Ambiguous bare tails (mapping to more than one known module) are skipped; downstream #1677 handling requires path-qualified names for those anyway. @@ -332,7 +389,7 @@ def _extract_issue_named_modules( for module in known: tail_to_modules.setdefault(module.rsplit("/", 1)[-1], []).append(module) - text = f"{title or ''}\n{body or ''}" + text = _issue_scan_text(title, body, comments) named: List[str] = [] seen: set[str] = set() @@ -343,15 +400,18 @@ def _extract_issue_named_modules( named.append(resolved) seen.add(resolved) - # Class 3: bare word-boundary mentions of multi-word (underscored) names. + # Class 3: bare word-boundary mentions of identifier-like names. for module in known: tail = module.rsplit("/", 1)[-1] - if module in seen or "_" not in tail or len(tail_to_modules[tail]) != 1: + if module in seen or len(tail_to_modules[tail]) != 1: + continue + if "_" not in tail and "-" not in tail: continue # No word chars, '/', '.', or '-' adjacent: matches a standalone - # mention but not longer identifiers (ci_validation_extra), path - # segments (a/ci_validation/b), or filenames (ci_validation.py is - # intentionally excluded — file mentions go through class 2). + # mention but not longer identifiers (ci_validation_extra, + # double-check-run), path segments (a/ci_validation/b), or filenames + # (ci_validation.py is intentionally excluded — file mentions go + # through class 2). if re.search(rf"(? 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 a module is - considered covered when either its full key or its final path component is - already present in the scope (by full key or tail). + 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 maps to exactly + one module in the known inventory. 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 (``extensions/a/src/page`` and + ``extensions/b/src/page``) 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 (PR #1983 review, P1b). 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] - scope_keys = set(scope_modules) | {_tail(m) for m in scope_modules} - return [ - m for m in issue_modules - if m not in scope_keys and _tail(m) not in scope_keys - ] + tail_counts: Dict[str, int] = {} + for module in _architecture_module_basenames(architecture or []): + tail = _tail(module) + tail_counts[tail] = tail_counts.get(tail, 0) + 1 + + 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 tail_counts.get(tail, 0) <= 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: @@ -2903,9 +2985,11 @@ def run_agentic_sync( # 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) + 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 + branch_modules, issue_named_modules, architecture ) if issue_only_modules: modules_to_sync = branch_modules + issue_only_modules From 60474450fc6ee454dcc310d746da211afad7ae11 Mon Sep 17 00:00:00 2001 From: sohni-tagirisa <248822596+sohni-tagirisa@users.noreply.github.com> Date: Fri, 10 Jul 2026 17:27:17 -0700 Subject: [PATCH 5/8] =?UTF-8?q?test(sync):=20red=20tests=20for=20round-3?= =?UTF-8?q?=20review=20findings=20=E2=80=94=20duplicate=20identities,=20ro?= =?UTF-8?q?ute=20brackets,=20comment/fence=20noise=20(#1980)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #1983 Codex round-2 review: - P1#1: duplicate-filename architecture entries (pdd_cloud Next.js page_TypeScriptReact.prompt routes) must stay distinct identities; prompt-path requests resolve path-qualified and a diff touching one sibling must not cover the other. - P1#2: bracketed dynamic-route segments ([id]) must survive prompt-path tokenization. - P2: comments contribute only trusted FILES_MODIFIED marker payloads (fenced markers are logs); class-3 bare-name matching ignores fenced code blocks in title/body. Refs #1980 Co-Authored-By: Claude Fable 5 --- tests/test_agentic_sync.py | 216 +++++++++++++++++++++++++++++++++++++ 1 file changed, 216 insertions(+) diff --git a/tests/test_agentic_sync.py b/tests/test_agentic_sync.py index 3595769d5..cc292005c 100644 --- a/tests/test_agentic_sync.py +++ b/tests/test_agentic_sync.py @@ -6211,3 +6211,219 @@ def test_union_path_qualified_duplicate_tail_not_masked( 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"] From e2e0323e11f3e751ab855ed9c2c5e21bac3ad4e4 Mon Sep 17 00:00:00 2001 From: sohni-tagirisa <248822596+sohni-tagirisa@users.noreply.github.com> Date: Fri, 10 Jul 2026 17:36:31 -0700 Subject: [PATCH 6/8] =?UTF-8?q?fix(sync):=20address=20round-3=20review=20?= =?UTF-8?q?=E2=80=94=20real=20identity=20index,=20bracketed=20paths,=20tru?= =?UTF-8?q?sted=20comment=20markers=20(#1980)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #1983 Codex round-2 review fixes: P1#1: extraction and coverage now consume _architecture_identity_index, which keeps one identity per distinct architecture entry (duplicates preserved; runtime *_LLM entries excluded; true duplicates deduped by (identity, filepath)) plus exact-match aliases from _architecture_entry_aliases. Tail-based resolution/coverage requires the tail to list exactly ONE real entry, so two Next.js page_TypeScriptReact.prompt routes no longer collapse to a single 'page' that lets a diff on one route mask an explicit request for another. A path-qualified key derived from a prompt path is accepted directly as a scheduler key (same form the branch-diff detector emits, #1675); a bare name colliding with a duplicated tail resolves to nothing. P1#2: _ISSUE_PROMPT_PATH_RE captures full non-whitespace .prompt paths, with leading markdown punctuation trimmed by _prompt_path_tokens (a leading '[' is stripped only when no matching ']' follows, preserving dynamic-route segments like app/users/[id]/page_TypeScriptReact.prompt). P2: comments contribute ONLY FILES_MODIFIED marker payloads — parsed with the change orchestrator's _extract_marker_paths walker after stripping fenced code blocks (a fenced marker is a quoted log, not a directive) — and only as prompt-path tokens. Class-3 bare-name matching in title/body now runs on fence-stripped text; an unclosed fence conservatively drops the remainder. Classes 1-2 stay global in title/body. Refs #1980 Co-Authored-By: Claude Fable 5 --- pdd/agentic_sync.py | 349 +++++++++++++++++++++++++++++++------------- 1 file changed, 247 insertions(+), 102 deletions(-) diff --git a/pdd/agentic_sync.py b/pdd/agentic_sync.py index de18aefe9..1ffb514c9 100644 --- a/pdd/agentic_sync.py +++ b/pdd/agentic_sync.py @@ -21,11 +21,13 @@ 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, build_dep_graph_from_architecture_data, ) @@ -230,8 +232,107 @@ def _detect_modules_from_branch_diff(project_root: Path) -> List[str]: # 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). -_ISSUE_PROMPT_PATH_RE = re.compile(r"[A-Za-z0-9_.\-][A-Za-z0-9_./\-]*\.prompt\b") +# 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 = "`\"'(<*,;:!]}>" + + +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. + """ + kept: List[str] = [] + fence_marker: Optional[str] = None + for line in text.splitlines(): + stripped = line.lstrip() + if fence_marker is None and ( + stripped.startswith("```") or stripped.startswith("~~~") + ): + fence_marker = stripped[:3] + continue + if fence_marker is not None and stripped.startswith(fence_marker): + fence_marker = None + continue + if fence_marker is None: + kept.append(line) + 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. True duplicate + entries (same primary identity AND same filepath) are counted once. + """ + identities: List[str] = [] + known_keys: set = set() + tail_to_identities: Dict[str, List[str]] = {} + seen_entries: set = set() + 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 + entry_key = (primary, filepath_str) + if entry_key in seen_entries: + continue + seen_entries.add(entry_key) + 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]: @@ -271,40 +372,26 @@ def _module_key_from_prompt_path(path_token: str) -> Optional[str]: return f"{prefix}{basename}" -def _issue_candidate_tokens(text: str) -> List[str]: - """Collect high-precision explicit-module tokens from issue text. - - Two token classes (see ``_extract_issue_named_modules``): - backticked single-word inline-code spans, and prompt-file path tokens - (FILES_MODIFIED-style lists). Prompt paths convert to path-qualified - module keys via ``_module_key_from_prompt_path``; runtime ``*_LLM.prompt`` - templates are rejected there and again in the downstream resolver. - """ - tokens: List[str] = [] - for raw in _ISSUE_BACKTICK_TOKEN_RE.findall(text): - token = raw.strip() - if token and not any(ch.isspace() for ch in token): - tokens.append(token) - for path_token in _ISSUE_PROMPT_PATH_RE.findall(text): - module_key = _module_key_from_prompt_path(path_token) - if module_key: - tokens.append(module_key) - return tokens - - def _resolve_issue_module_token( token: str, - known_set: set, - tail_to_modules: Dict[str, List[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. A candidate that is not an exact known key may still resolve - through its final path component when that tail is UNAMBIGUOUS (exactly - one known module) 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). + 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 @@ -315,9 +402,11 @@ def _resolve_issue_module_token( if stripped: candidates.append(stripped) for cand in candidates: - if cand in known_set: - return cand - tail_matches = tail_to_modules.get(cand.rsplit("/", 1)[-1], []) + 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}") @@ -327,21 +416,96 @@ def _resolve_issue_module_token( return None -def _issue_scan_text( - title: str, - body: str, - comments: Optional[List[Dict[str, Any]]], -) -> str: - """Join the issue title, body, and comment BODIES for module scanning. +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. """ - parts = [title or "", body or ""] + paths: List[str] = [] for comment in comments or []: - if isinstance(comment, dict): - parts.append(str(comment.get("body", "") or "")) - return "\n".join(parts) + 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( @@ -354,11 +518,10 @@ def _extract_issue_named_modules( Issue #1980: the branch-diff fast path must not silently drop modules the issue explicitly requests. This helper matches the issue title/body — plus - the FILTERED comment bodies (the same signal-bearing set the identify path - consumes; FILES_MODIFIED markers arrive as comments, PR #1983 review P1a) - — against the KNOWN module inventory (architecture basenames, runtime - ``*_LLM`` templates already excluded by ``_architecture_module_basenames``) - using three high-precision, zero-LLM token classes: + 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 @@ -374,48 +537,32 @@ def _extract_issue_named_modules( plain-prose mention of a single-word module is deliberately NOT treated as an explicit request; use backticks or the prompt path). - Ambiguous bare tails (mapping to more than one known module) are skipped; - downstream #1677 handling requires path-qualified names for those anyway. - Returns known-module keys in first-mention order; empty when there is no + Scan surfaces (PR #1983 round 3, P2): classes 1 and 2 scan the title/body; + class 3 scans the title/body with fenced code blocks stripped (quoted + logs/snippets are not requests). Comments contribute ONLY trusted + ``FILES_MODIFIED:`` marker payloads (class 2), 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. """ - known = _architecture_module_basenames(architecture or []) - if not known: + identities, known_keys, tail_to_identities = _architecture_identity_index( + architecture + ) + if not identities: return [] - known_set = set(known) - # Map each final path component to its full module keys so a bare mention - # of "worker_app" can resolve "extensions/foo/src/worker_app". - tail_to_modules: Dict[str, List[str]] = {} - for module in known: - tail_to_modules.setdefault(module.rsplit("/", 1)[-1], []).append(module) - - text = _issue_scan_text(title, body, comments) - named: List[str] = [] - seen: set[str] = set() - - # Classes 1 + 2: backticked tokens and prompt-file paths. - for token in _issue_candidate_tokens(text): - resolved = _resolve_issue_module_token(token, known_set, tail_to_modules) - if resolved and resolved not in seen: - named.append(resolved) - seen.add(resolved) - # Class 3: bare word-boundary mentions of identifier-like names. - for module in known: - tail = module.rsplit("/", 1)[-1] - if module in seen or len(tail_to_modules[tail]) != 1: - continue - if "_" not in tail and "-" not in tail: - continue - # No word chars, '/', '.', or '-' adjacent: matches a standalone - # mention but not longer identifiers (ci_validation_extra, - # double-check-run), path segments (a/ci_validation/b), or filenames - # (ci_validation.py is intentionally excluded — file mentions go - # through class 2). - if re.search(rf"(? str: return module.rsplit("/", 1)[-1] - tail_counts: Dict[str, int] = {} - for module in _architecture_module_basenames(architecture or []): - tail = _tail(module) - tail_counts[tail] = tail_counts.get(tail, 0) + 1 + _, _, tail_to_identities = _architecture_identity_index(architecture) scope_full = set(scope_modules) scope_tails = {_tail(m) for m in scope_modules} @@ -457,7 +602,7 @@ def _covered(module: str) -> bool: if module in scope_full: return True tail = _tail(module) - return tail_counts.get(tail, 0) <= 1 and tail in scope_tails + return len(tail_to_identities.get(tail, [])) <= 1 and tail in scope_tails return [m for m in issue_modules if not _covered(m)] From 1cfe8a62f8b83cf3000793c2bdbbf900da7dfea5 Mon Sep 17 00:00:00 2001 From: sohni-tagirisa <248822596+sohni-tagirisa@users.noreply.github.com> Date: Sun, 12 Jul 2026 15:38:20 -0700 Subject: [PATCH 7/8] =?UTF-8?q?test(sync):=20red=20tests=20for=20round-4?= =?UTF-8?q?=20review=20findings=20=E2=80=94=20cross-project=20collapse,=20?= =?UTF-8?q?dep=20edges,=20fence=20handling=20(#1980)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #1983 Codex round-3 review: - P1#1: string-identical entries from different nested projects (the combined architecture carries no source qualification) must stay ambiguous, never merge into one falsely-unique identity. - P1#2: prompt-subpath scheduler keys must keep their entry's declared dependencies in the targeted dep graph (silent empty-deps repro), with an ambiguous-suffix no-guess guard. - P2#1: fenced issue-body logs must not widen scope via class-1/2 tokens. - P2#2: 4+ char fences must strip as one block (CommonMark close rule). Refs #1980 Co-Authored-By: Claude Fable 5 --- tests/test_agentic_sync.py | 149 +++++++++++++++++++++++++++++++++++++ 1 file changed, 149 insertions(+) diff --git a/tests/test_agentic_sync.py b/tests/test_agentic_sync.py index cc292005c..4436f3c0e 100644 --- a/tests/test_agentic_sync.py +++ b/tests/test_agentic_sync.py @@ -6427,3 +6427,152 @@ def test_union_duplicate_filename_prompt_path_not_masked( 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"] From f680c761462160ecdaec7b550be26ea529024801 Mon Sep 17 00:00:00 2001 From: sohni-tagirisa <248822596+sohni-tagirisa@users.noreply.github.com> Date: Sun, 12 Jul 2026 15:44:26 -0700 Subject: [PATCH 8/8] =?UTF-8?q?fix(sync):=20address=20round-4=20review=20?= =?UTF-8?q?=E2=80=94=20no=20identity=20dedupe,=20dep-graph=20target=20reso?= =?UTF-8?q?lution,=20CommonMark=20fences=20(#1980)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #1983 Codex round-3 review fixes: P1#1: _architecture_identity_index no longer dedupes string-identical entries. load_combined_architecture_data concatenates nested architecture files with no source-project qualification, so two identical entries may be distinct modules in different projects; treating them as ambiguous (no bare resolution, no tail coverage) is the only safe deterministic choice. Recall cost is confined to a module literally double-listed with identical strings. P1#2: _build_targeted_dep_graph pre-resolves each group target via _dep_graph_target_for_module — a prompt-subpath scheduler key (app/settings/page) that fails exact alias matching but path-suffix matches exactly ONE entry's qualified alias is rewritten to that alias for graph construction, then the graph is remapped back to the original scheduler keys. Declared dependencies of widened (and plain branch-diff) keys against flat-filename entries are no longer silently dropped; the bare filename alias is never used and ambiguous suffixes stay unchanged (no-guess). Scheduler keys themselves are untouched, preserving exact coverage against diff keys and child-sync acceptance. P2#1: title/body is fence-stripped once, before ALL three extraction classes (previously only class-3 bare-name matching). P2#2: the fence stripper tracks the opening fence character and length and closes only on a run of the same character at least as long (CommonMark), so 4+ char fences swallow inner triple fences as quoted content. Refs #1980 Co-Authored-By: Claude Fable 5 --- pdd/agentic_sync.py | 125 +++++++++++++++++++++++++++++++++----------- 1 file changed, 94 insertions(+), 31 deletions(-) diff --git a/pdd/agentic_sync.py b/pdd/agentic_sync.py index 1ffb514c9..35029b9c6 100644 --- a/pdd/agentic_sync.py +++ b/pdd/agentic_sync.py @@ -29,6 +29,7 @@ _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 @@ -244,6 +245,8 @@ def _detect_modules_from_branch_diff(project_root: Path) -> List[str]: # 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]: @@ -265,21 +268,27 @@ def _strip_fenced_code_blocks(text: str) -> str: 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_marker: Optional[str] = None + fence_char: Optional[str] = None + fence_len = 0 for line in text.splitlines(): - stripped = line.lstrip() - if fence_marker is None and ( - stripped.startswith("```") or stripped.startswith("~~~") - ): - fence_marker = stripped[:3] - continue - if fence_marker is not None and stripped.startswith(fence_marker): - fence_marker = None + 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 fence_marker is None: - kept.append(line) + 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) @@ -305,13 +314,20 @@ def _architecture_identity_index( * ``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. True duplicate - entries (same primary identity AND same filepath) are counted once. + 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]] = {} - seen_entries: set = set() for entry in architecture or []: if not isinstance(entry, dict): continue @@ -325,10 +341,6 @@ def _architecture_identity_index( primary = _basename_from_architecture_filepath(filepath_str) if not primary: continue - entry_key = (primary, filepath_str) - if entry_key in seen_entries: - continue - seen_entries.add(entry_key) identities.append(primary) known_keys.update(_architecture_entry_aliases(entry)) tail_to_identities.setdefault(primary.rsplit("/", 1)[-1], []).append(primary) @@ -537,10 +549,10 @@ def _extract_issue_named_modules( 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 round 3, P2): classes 1 and 2 scan the title/body; - class 3 scans the title/body with fenced code blocks stripped (quoted - logs/snippets are not requests). Comments contribute ONLY trusted - ``FILES_MODIFIED:`` marker payloads (class 2), never free text. + 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 @@ -554,13 +566,14 @@ class 3 scans the title/body with fenced code blocks stripped (quoted if not identities: return [] - text = f"{title or ''}\n{body or ''}" + # 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( - _strip_fenced_code_blocks(text), identities, tail_to_identities - ): + ) + _bare_mention_identities(text, identities, tail_to_identities): if module not in named: named.append(module) return named @@ -2268,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], @@ -2306,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