fix: pdd-change Step 6 should use <pdd-dependency> tags, not <include> tags - #2376
fix: pdd-change Step 6 should use <pdd-dependency> tags, not <include> tags#2376agarwal-ishaan wants to merge 19 commits into
Conversation
|
/heal |
gltanaka
left a comment
There was a problem hiding this comment.
Thanks for consolidating the earlier prompt and implementation PRs. The architecture-based direction is sensible, but this version has two merge-blocking correctness problems:
-
architecture.json.dependenciesis not currently a<pdd-dependency>-only data source.merge_auto_deps_includes_into_architecture()intentionally writes module/example<include>targets into the same array, and existing tests require that behavior. Consequently, reading this field cannot prove that an include-only module edge is excluded. The new Step 6 test hand-writes JSON without the include edge, so it does not exercise the real prompt → architecture → Step 6 path. -
build_dependency_graph_from_architecture()usesextract_module_from_include()for architecture identities. That helper drops directories and rejects_LLM.promptfiles. This loses real declared dependencies in the current repository. For example,commands/gate_python.prompt -> gate_python.promptcollapses togate -> gateand is discarded as a self-edge; dependencies such asagentic_update_python.prompt -> agentic_update_LLM.promptare also omitted.
Please resolve the dependency provenance/semantics and use a path-preserving architecture identity that supports LLM modules. Add regressions for:
- a real prompt-to-architecture-to-Step-6 flow distinguishing
<pdd-dependency>from context-only<include>; - path-qualified modules with the same basename;
_LLM.promptdependencies;- root plus nested architecture files.
The focused suites pass (317 tests), but they cover only flat _python.prompt names and do not exercise these production shapes. Follow-up scope should also address the issue's requested prompting-guide examples and the retained top-30/top-10 truncation.
…t architecture.json's mixed dependencies field Addresses both merge-blocking issues from PR promptdriven#2376 review: - architecture.json's `dependencies` field is not <pdd-dependency>-only (merge_auto_deps_includes_into_architecture() also writes <include>-derived edges into it), so build_dependency_graph_from_architecture() no longer reads it directly. It now uses architecture.json only to enumerate modules and locate each one's prompt file, then parses <pdd-dependency> tags fresh from that file via the existing architecture_sync helpers. - extract_module_from_include() dropped directory prefixes and rejected _LLM.prompt files, causing real collisions/dropped edges in this repo's own architecture.json (e.g. commands/gate_python.prompt colliding with gate_python.prompt as a false self-edge). Replaced with a new _architecture_module_key() that preserves path and LLM-prompt identity. Replaced the hand-written-JSON tests the reviewer flagged with real prompt-file fixtures covering all four requested regression scenarios: pdd-dependency vs. include-only, path-qualified same-basename collision, _LLM.prompt dependencies, and nested architecture.json files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Implemented the above changes, requesting you to please review |
…not <include> Step 6's _build_dependency_context() currently builds its module-dependency graph by scanning <include> tags (build_dependency_graph()), but <include> tags are LLM context, not declared architectural dependencies. A module can truly depend on another via <pdd-dependency> without ever <include>-ing it, and can <include> another module's example purely for style without depending on it at all -- so the current graph both misses real dependents and fabricates false ones. Updates the source prompts (not the generated code yet) to specify a new sync_order.build_dependency_graph_from_architecture() that reads architecture.json's `dependencies` field (already populated from <pdd-dependency> tags by architecture_sync.py) and wires it into Step 6's dependency context instead. Fixes promptdriven#1807 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… not pseudocode Restyles the two sections added for promptdriven#1807 (sync_order's 3a and agentic_change_orchestrator's Dependency Context section) to state the required behavior in prose, matching the prompting guide's requirements-as- contract convention, instead of an imperative step-by-step algorithm.
Implements the function specified in pdd/prompts/sync_order_python.prompt (PR promptdriven#2373): builds the module dependency graph from architecture.json's `dependencies` field (populated from <pdd-dependency> tags) instead of scanning <include> tags, matching what pdd-change Step 6 needs per promptdriven#1807. Generated via `pdd generate --incremental`. Adds unit tests covering the bare-array and {"modules": [...]} architecture.json formats, and missing/ invalid-file handling.
_build_dependency_context() now takes architecture_path and calls build_dependency_graph_from_architecture() (added in promptdriven#2375) instead of the include-based build_dependency_graph(prompts_dir). This is the actual Step 6 behavior fix from promptdriven#1807: the dependency graph shown to the LLM now reflects declared <pdd-dependency> tags (via architecture.json) instead of <include> tags, so it no longer misses real dependents or fabricates false ones from stylistic includes. Matches pdd/prompts/agentic_change_orchestrator_python.prompt (promptdriven#2373) exactly. Adds 3 unit tests covering: a <pdd-dependency> edge surfacing correctly with no matching <include>, missing architecture.json, and an empty graph.
…xt prompt Drops the leftover implementation-step narration (which helper function to call, exact variable assignment) in favor of stating the contract: what the summary contains, where it's stored, and when it's empty.
… spec Reverts to the file's established Helper: func_sig: - bullet convention, changing only what the fix actually requires (signature, the function call, the existence check) instead of rephrasing the whole section into prose.
Trims wording (drops "Builds"/"Accepts...as either"/redundant phrasing) to match the file's established telegraphic bullet style, same substance.
…ction Merges the shape/naming-reuse and format-acceptance/self-dep bullets into one; same substance, less bulk for a ~20-line function.
Replaces "same shape as build_dependency_graph()" with the actual shape description, and drops the "NOT build_dependency_graph(prompts_dir)" comparison (the surrounding paragraph already establishes that contrast). Each function's contract should stand on its own without requiring the reader to cross-reference a sibling function's spec.
…t architecture.json's mixed dependencies field Addresses both merge-blocking issues from PR promptdriven#2376 review: - architecture.json's `dependencies` field is not <pdd-dependency>-only (merge_auto_deps_includes_into_architecture() also writes <include>-derived edges into it), so build_dependency_graph_from_architecture() no longer reads it directly. It now uses architecture.json only to enumerate modules and locate each one's prompt file, then parses <pdd-dependency> tags fresh from that file via the existing architecture_sync helpers. - extract_module_from_include() dropped directory prefixes and rejected _LLM.prompt files, causing real collisions/dropped edges in this repo's own architecture.json (e.g. commands/gate_python.prompt colliding with gate_python.prompt as a false self-edge). Replaced with a new _architecture_module_key() that preserves path and LLM-prompt identity. Replaced the hand-written-JSON tests the reviewer flagged with real prompt-file fixtures covering all four requested regression scenarios: pdd-dependency vs. include-only, path-qualified same-basename collision, _LLM.prompt dependencies, and nested architecture.json files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This reverts commit 9dfb027.
1737e68 to
8c4338c
Compare
gltanaka
left a comment
There was a problem hiding this comment.
Re-review of current head 8c4338c9 against origin/main (f25a0139), including the complete PR/issue history, superseded PRs #2373/#2375, all commits, the cumulative diff, architecture.json, prompt sources, prompting guide, and the PR-loop runbook. There are no inline review threads (0 resolved / 0 unresolved). The core provenance correction is directionally right, but this is not merge-ready.
Blocking findings
-
P1 — the verification-profile transition fails on the actual PR base.
pdd/sync_core/verification.py:2782validates the protected active requirement rotations before it can apply the candidate retirement added in.pdd/verification-profile-rotations.json:676. On this exact head, the production path
load_verification_profiles(build_unit_manifest(root, base_ref="origin/main", head_ref="HEAD"))
raisesVerificationProfileError: protected active requirement transition rules are ambiguousbecause protectedorigin/mainhas two activecode_generator_main_python.prompt/pythonrows. The current hash-only reconciliation test does not exercise the real base-to-head load.Please either land/rebase onto the prerequisite protected retirement and remove the duplicate candidate retirement, or add a narrowly byte-bound bridge that can validate this exact transition before protected-row uniqueness is enforced. Add the exact
origin/main -> PR headproduction-loader regression. -
P1 — Step 6 still drops most real dependency context.
pdd/agentic_change_orchestrator.py:2284-2290retains the old[:30]module and[:10]dependent slices, andpdd/prompts/agentic_change_orchestrator_python.prompt:329explicitly requires them. Against this repository's real architecture-derived graph, 135 dependency targets have dependents, but only 30 are rendered: 105 targets and 8 additional dependent names disappear. In particular, the newly preservedagentic_update -> agentic_update_LLMedge exists in the graph, butagentic_update_LLMranks 68th and is absent from the actual Step 6 context. Thus the prior_LLMomission remains at the user-visible boundary.Render all edges in a compact deterministic representation (or another complete representation with explicit fidelity guarantees) and add >30-target and >10-dependent regressions at
_build_dependency_context, not only at the graph helper. -
P1 — nested architecture support is only partially implemented and the requested real flow is still untested.
pdd/sync_order.py:244,261assumes prompt files live under the architecture file's ownprompts/orpdd/prompts/. That contradicts the established contract inpdd/architecture_sync.py:118-138/ its tests: a nestedarchitecture.jsonmay use an ancestor project'sprompts/directory. A deterministic reproduction withrepo/prompts/auth_python.promptandrepo/backend/functions/architecture.jsonreturns{'auth': [], 'log': []}and warnings instead ofauth -> log. The valid root entrycontext/python_preamble.promptis likewise misresolved aspdd/prompts/context/python_preamble.prompt.The advertised “real prompt -> architecture” tests do not cover this boundary:
tests/test_sync_order.py:827-850says it does not hand-write architecture JSON, but lines 842-848 do exactly that;tests/test_agentic_change_orchestrator.py:10784does the same. Neither runs architecture sync/auto-deps, where include-derived edges makearchitecture.json.dependenciesmixed-provenance.Resolve prompts through the governing/nearest supported prompt root (and safe architecture-relative paths), then add a deterministic production-flow regression that creates architecture state through sync/auto-deps and invokes Step 6. Cover root and nested architectures, including the ancestor-prompts shape.
-
P2 — the ancillary include-parser fix still treats fenced examples as directives.
pdd/sync_core/includes.py:211and:264skip only a tag whose immediately preceding character is a backtick.<include>and<include-many>inside triple-backtick or~~~fenced examples are still returned as real references, contrary todocs/prompting_guide.md:1404; this can make closure validation/fingerprinting require example-only files. The new tests cover inline code only.Exclude both Markdown fence styles while preserving the supported legacy
<path>include grammar, and add fenced XML/include-many regressions. -
P2 — the linked issue's prompting-guide acceptance work remains absent.
#1807 explicitly asks for practical examples explaining when a real module relationship uses
<pdd-dependency>and when files are only LLM context via<include>. This PR does not changedocs/prompting_guide.md. Its current line 1645 also says<include>“does NOT affect architecture,” while the implementation and this PR correctly acknowledge that auto-deps can write include-derived edges into the architecturedependenciesarray.Add the requested customer-service/payment-status versus refund-policy/tone-context examples and reconcile that mixed-field behavior in the guide. If that work is intentionally deferred, this PR should not close #1807; link a narrower issue instead.
Prior-request checklist
| Prior request | Implementing evidence | Current status |
|---|---|---|
| Stop Step 6 using the include graph | 4b6349dfa |
satisfied |
Do not trust mixed architecture.json.dependencies as pdd-dependency-only |
626002541, fresh prompt-tag parsing |
satisfied |
| Preserve path-qualified same-basename identities | 626002541; tests/test_sync_order.py:877-895 |
satisfied |
Preserve _LLM.prompt dependencies |
626002541; graph test at :898-914 |
partially satisfied — graph keeps it, Step 6 truncates it |
| Root + nested architecture behavior | 626002541; local-prompts-only test at :917-935 |
partially satisfied — supported ancestor prompt roots fail |
| Real prompt -> architecture -> Step 6 regression | claimed by 626002541 |
not addressed — tests hand-write architecture JSON |
| Prompting-guide distinction and examples | no implementing commit/diff | not addressed |
| Remove top-30/top-10 loss | no implementing commit; slices remain | not addressed |
| Verification-profile reconciliation | 849fdd5ac, 460998285, 7abac24eb |
regressed/blocking — production base-to-head load fails |
| Ignore documentation examples in include parsing | 8c4338c97 |
partially satisfied — inline works, fenced examples regress |
| Initial direct architecture-field design / first restamp | superseded by 626002541; 0181bd446 reverted by 315bd833 |
obsolete with valid supersession |
Validation evidence: 323 focused orchestrator/sync-order tests passed; 21 architecture/auto-deps boundary tests passed; GitHub's Unit Tests, Public CLI Regression, Story Regression, Package Preprocess Smoke, Repo Bloat Docker E2E, and heal checks are green. Those green checks do not cover the failing production profile load or the deterministic Step 6/nested/fence reproductions above.
Summary
pdd changeStep 6 built its module-dependency context by scanning<include>tags (build_dependency_graph()), but<include>tags are LLM context, not declared architectural dependencies. A module can truly depend on another via<pdd-dependency>without ever<include>-ing it, and can<include>another module's example purely for style without depending on it at all -- so the old graph both missed real dependents and fabricated false ones.pdd/prompts/sync_order_python.prompt,pdd/prompts/agentic_change_orchestrator_python.prompt): specify a newbuild_dependency_graph_from_architecture()function and wire Step 6's dependency context to use it.pdd/sync_order.py: implementsbuild_dependency_graph_from_architecture(architecture_path) -> Dict[str, List[str]], readingarchitecture.json'sdependenciesfield (populated from<pdd-dependency>tags byarchitecture_sync.py) instead of scanning<include>tags. Generated viapdd generate --incremental.pdd/agentic_change_orchestrator.py:_build_dependency_context()now takesarchitecture_pathand calls the function above instead of the include-basedbuild_dependency_graph(prompts_dir)-- the actual Step 6 behavior fix.Test plan
pytest tests/test_sync_order.py-- 52 passedpytest tests/test_agentic_change_orchestrator.py-- 265 passedpdd-issue-1807-repro/):_build_dependency_context()now correctly showspayment_status -> affects: customer_service(previously missing) and no longer fabricateslegacy_wizard -> affects: onboardingagentic_sync,sync_graph_order_consistency) that also import fromsync_orderstill import cleanlyFixes #1807