Feature/733 sync dependency graph + arch dependencies#736
Feature/733 sync dependency graph + arch dependencies#736vishalramvelu wants to merge 10 commits intopromptdriven:mainfrom
Conversation
|
target 3/30 |
There was a problem hiding this comment.
Pull request overview
This PR enhances dependency-aware behavior in agentic/architecture sync by adding richer dependency-graph results (graph + warnings) and preventing unintended dependency wipes during architecture sync. It also expands repository-wide pdd update behavior with dry-run reporting, budget caps, and post-update regeneration steps.
Changes:
- Make
build_dep_graph_from_architecturereturn aDepGraphFromArchitectureResult(graph + warnings) and surface orphan/partial-sync warnings inagentic_sync. - Update
update_architecture_from_promptto only modifydependencieswhen explicit<pdd-dependency>intent is present (including empty tags to clear). - Extend repo-wide
pdd updatewith--all,--dry-run,--budget, dependency ordering, drift reporting, and post-update example/doc regeneration + autosubmit hooks.
Reviewed changes
Copilot reviewed 16 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
pdd/agentic_sync_runner.py |
Introduces DepGraphFromArchitectureResult and emits validation warnings while building the dependency subgraph. |
pdd/agentic_sync.py |
Prints graph-builder warnings in non-quiet runs and adapts to new return type. |
pdd/architecture_sync.py |
Preserves architecture dependencies unless <pdd-dependency> metadata is explicitly present. |
pdd/update_main.py |
Adds repo-wide dry-run drift reporting, budget caps, dependency ordering, and post-update regeneration/submission steps. |
pdd/commands/modify.py |
Adds pdd update flags --all, --budget, --dry-run and enforces mode-specific validation. |
tests/test_agentic_sync_runner.py |
Updates/extends coverage for graph builder warnings and new return type. |
tests/test_agentic_sync.py |
Updates mocks/assertions for DepGraphFromArchitectureResult integration. |
tests/test_architecture_sync.py |
Updates tests to validate dependency preservation/clearing semantics. |
tests/test_update_main.py |
Adds coverage for drift-report doc counting, .mdx exclusion, dry-run/budget behavior, dependency ordering, and post-update hooks. |
tests/test_update_command.py |
Adds CLI coverage for --all, --dry-run, and --budget option handling and validation. |
tests/test_sync_code_main.py |
Updates assertions for new create_missing_prompts behavior when scanning. |
pdd/prompts/core_dump_requirements_LLM.prompt |
Adds/updates prompt spec content for core dump requirements. |
pdd/prompts/agentic_e2e_fix_orchestrator_python.prompt |
Adds convergence guard requirements to the orchestrator prompt. |
project_dependencies.csv |
Adds a repo dependency summary CSV artifact. |
examples/prompts_linter/project_dependencies.csv |
Removes example dependency CSV. |
examples/hello/project_dependencies.csv |
Removes example dependency CSV. |
context/agentic_sync_runner_example.py |
Updates example usage for new dep-graph return type. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gltanaka
left a comment
There was a problem hiding this comment.
Hey Vishal, nice work on this PR! The dep graph warnings (Area 1) and the dependency preservation fix (Area 2) are solid — those are real bugs you've identified and the fixes are clean. The pdd update --all with dry-run and dependency ordering is a useful addition too. Tests look thorough.
A few things to clean up before we merge:
-
Remove
uv.lock(+3,637 lines) — The repo doesn't currently use uv, so adding a lock file here is a big infrastructure decision that affects all contributors. Let's handle that separately if we decide to adopt uv. -
Remove
project_dependencies.csv(+258 lines) — This is an auto-generated artifact that will go stale and cause merge conflicts as soon as anyone else merges. Either remove it from the PR or add it to.gitignore. -
_estimate_dry_run_cost_rangeunused params — I seectx,repo_obj, andsimpleare still in the signature even though you mentioned dropping them in your reply to the Copilot review. Looks like the fix didn't make it into a commit — just need to clean those up.
Everything else looks good. These are quick fixes — looking forward to merging this once they're in!
gltanaka
left a comment
There was a problem hiding this comment.
Updating my earlier review — I was wrong about project_dependencies.csv, that's a legitimate PDD auto-deps artifact. Ignore that point. Apologies for the noise!
Revised asks (just two):
-
Remove
uv.lock(+3,637 lines) — The repo doesn't currently use uv, so adding a lock file here is a big infrastructure decision that affects all contributors. Let's handle that separately if we decide to adopt uv. -
_estimate_dry_run_cost_rangeunused params — I seectx,repo_obj, andsimpleare still in the signature even though you mentioned dropping them in your reply to the Copilot review. Looks like the fix didn't make it into a commit — just need to clean those up.
Everything else looks good — the dep graph warnings, dep preservation fix, and pdd update --all features are solid work. Quick fixes and we can merge!
Summary
Improves agentic / architecture sync behavior around architecture.json dependencies:
Dependency graph visibility - build_dep_graph_from_architecture now returns DepGraphFromArchitectureResult (graph + warnings) instead of a bare dict. Warnings cover:
Orphan dependency filenames (no matching module entry in architecture.json)
Partial sync: dependency resolves to a module not in the sync target set (edge still omitted from the scheduled subgraph)
agentic_sync prints these as yellow Warning: lines when not in --quiet mode.
Preserve arch dependencies unless the prompt defines them -
update_architecture_from_prompt updates dependencies only when the prompt carries dependency intent: present (including empty tag to clear) or a non-empty parsed dependency list. Reason- / interface-only prompts no longer wipe existing dependencies.
pdd updaterepo-wide mode (if in this PR)--all,--budget,--dry-run(plus existing repo options:--extensions,--directory,--base-branch, etc.).--dry-run: drift report and estimated cost range without LLM calls or writes.Tests
uv run pytest tests/test_agentic_sync_runner.py tests/test_agentic_sync.py tests/test_architecture_sync.py -v
Manual Tests
PR1 — REPL / one-off script (from repo root, uv run python):
Create a temp architecture.json with:
one entry whose dependencies include a nonexistent filename → expect an orphan warning;
one entry depending on a module outside the target basename list → partial sync warning;
Call build_dep_graph_from_architecture(path, ["mod_a", "mod_b"]) (adjust names) and inspect result.graph and result.warnings.
Checklist
Change #735