From bc693fd1c8871e7d2f467056e9d4ba6a61534e5e Mon Sep 17 00:00:00 2001 From: coherent-cache Date: Tue, 10 Mar 2026 16:05:07 +0530 Subject: [PATCH] Fix sync_decisions and _run_modify skipping directory spec_paths When spec_paths contains a directory (e.g., "specs/"), sync_decisions silently skips it because `spec_path.is_file()` returns False for directories. This means approved decisions are never synced to spec files. The same bug exists in _run_modify in cli.py. parse_spec_files already handles directories correctly by expanding them via rglob("*.md"). This commit applies the same resolution pattern to sync_decisions and _run_modify. Adds test fixture for directory-based spec_paths and two tests confirming sync and parse both work with directory spec_paths. --- plumb/cli.py | 7 +++++-- plumb/sync.py | 13 +++++++++++-- tests/conftest.py | 21 ++++++++++++++++++++ tests/test_sync.py | 48 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 85 insertions(+), 4 deletions(-) diff --git a/plumb/cli.py b/plumb/cli.py index 253f012..d307e43 100644 --- a/plumb/cli.py +++ b/plumb/cli.py @@ -484,13 +484,16 @@ def _run_modify(repo_root: Path, decision_id: str) -> None: console.print(" [yellow]No staged changes to modify.[/yellow]") return - # Read spec + # Read spec (resolve directories to individual .md files) config = load_config(repo_root) spec_content = "" if config: for sp in config.spec_paths: spec_file = repo_root / sp - if spec_file.is_file(): + if spec_file.is_dir(): + for md_file in sorted(spec_file.rglob("*.md")): + spec_content += md_file.read_text() + elif spec_file.is_file(): spec_content += spec_file.read_text() decision_branch = find_decision_branch(repo_root, decision_id) diff --git a/plumb/sync.py b/plumb/sync.py index 076081e..7832208 100644 --- a/plumb/sync.py +++ b/plumb/sync.py @@ -267,10 +267,19 @@ def sync_decisions( ) decisions_text = "\n".join(decision_lines) + # Resolve spec paths: directories expand to all .md files within + # (matches the resolution logic in parse_spec_files above) + resolved_spec_files: list[Path] = [] for spec_path_str in config.spec_paths: - if on_progress: - on_progress(f"Updating spec: {spec_path_str}...") spec_path = repo_root / spec_path_str + if spec_path.is_dir(): + resolved_spec_files.extend(sorted(spec_path.rglob("*.md"))) + elif spec_path.is_file(): + resolved_spec_files.append(spec_path) + + for spec_path in resolved_spec_files: + if on_progress: + on_progress(f"Updating spec: {spec_path.relative_to(repo_root)}...") if not spec_path.is_file(): continue diff --git a/tests/conftest.py b/tests/conftest.py index 2f871e5..08a5795 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -43,6 +43,27 @@ def initialized_repo(tmp_repo): return tmp_repo +@pytest.fixture +def initialized_repo_dir_specs(tmp_repo): + """A tmp_repo with spec_paths pointing to a directory (not individual files).""" + ensure_plumb_dir(tmp_repo) + cfg = PlumbConfig( + spec_paths=["specs/"], + test_paths=["tests/"], + initialized_at=datetime.now(timezone.utc).isoformat(), + ) + save_config(tmp_repo, cfg) + # Create specs directory with multiple spec files + specs_dir = tmp_repo / "specs" + specs_dir.mkdir(exist_ok=True) + (specs_dir / "spec.md").write_text("# Spec\n\n## Features\n\nThe system must do X.\n") + (specs_dir / "api.md").write_text("# API\n\n## Endpoints\n\nGET /items returns all items.\n") + # Create tests dir + (tmp_repo / "tests").mkdir(exist_ok=True) + (tmp_repo / ".plumb" / "decisions").mkdir(exist_ok=True) + return tmp_repo + + @pytest.fixture def sample_decisions(): """Return a list of sample Decision objects.""" diff --git a/tests/test_sync.py b/tests/test_sync.py index edd3b0e..1427230 100644 --- a/tests/test_sync.py +++ b/tests/test_sync.py @@ -189,6 +189,54 @@ def test_new_requirement_gets_current_timestamp(self, initialized_repo): assert result[0]["last_seen_commit"] is None +class TestSyncDecisionsDirectorySpecPaths: + """Tests for directory-based spec_paths (e.g., spec_paths: ['specs/']).""" + + def test_syncs_when_spec_path_is_directory(self, initialized_repo_dir_specs): + """sync_decisions should resolve directory spec_paths to .md files.""" + d = Decision( + id="dec-dir1", + status="approved", + question="How to authenticate?", + decision="Use API keys.", + created_at=datetime.now(timezone.utc).isoformat(), + ) + append_decision(initialized_repo_dir_specs, d, branch="main") + + updater_call_count = [0] + + def mock_run(fn, *args, **kwargs): + from plumb.programs.spec_updater import WholeFileSpecUpdater + if isinstance(fn, WholeFileSpecUpdater): + updater_call_count[0] += 1 + return [{"header": "## Features", "content": "Uses API keys.\n"}], [] + return [] + + with patch("plumb.programs.configure_dspy"), \ + patch("plumb.programs.run_with_retries", side_effect=mock_run): + result = sync_decisions(initialized_repo_dir_specs) + + # Should have called updater for each .md file in the directory + assert updater_call_count[0] == 2 # spec.md + api.md + assert result["spec_updated"] >= 1 + + def test_parses_spec_files_from_directory(self, initialized_repo_dir_specs): + """parse_spec_files should find .md files inside directory spec_paths.""" + mock_reqs = [ + MagicMock(text="The system must do X.", ambiguous=False), + ] + + with patch("plumb.programs.configure_dspy"), \ + patch("plumb.programs.run_with_retries", return_value=mock_reqs): + result = parse_spec_files(initialized_repo_dir_specs) + + # Should parse both spec.md and api.md (2 files × 1 req each = 2, but + # same text → same ID → deduped to 1) + assert len(result) >= 1 + req_path = initialized_repo_dir_specs / ".plumb" / "requirements.json" + assert req_path.exists() + + class TestSyncDecisionsWholeFile: """Tests for the whole-file spec update path."""