diff --git a/CannedPromptFormat.md b/CannedPromptFormat.md index 665e4a6..6b4d93c 100644 --- a/CannedPromptFormat.md +++ b/CannedPromptFormat.md @@ -741,6 +741,60 @@ that the including package does not supply is an error naming both packages. Implementations MUST detect inclusion cycles and report them rather than recursing. +#### Inclusion is textual and repeated + +Inclusion is not deduplicated, and there are two ways the same text ends up +rendered twice. + +**Two inputs include the same dependency.** The values are equal and both are +substituted: + +```text +review/change +├── style → include style/house +└── preamble → include style/house ← the same text again +``` + +**An included package inherits an outer input of the same name.** This one is +easy to miss, because nothing was included twice: + +```text +review/change +├── conventions → include team/conventions ← rendered here +└── routine → include review/commit-routine + └── {{ conventions }} ← inherited (§ 10.4), rendered again +``` + +`review/commit-routine` declares its own `conventions` input. When it is +composed, resolution passes the including package's already-resolved values +down by name, so it inherits the outer text rather than resolving its own — and +renders it a second time. No dependency was included twice; the text still +appears twice. + +Nothing is wrong with either package. The duplication is a property of how they +were composed. + +Deduplication is deliberately not specified. It would require deciding which +occurrence survives — position in a prompt carries meaning — and what happens +when the two paths select *different versions* of the same dependency (§ 10.3 +permits that: one path may pin `1.0.0` while another asks for `newest`). +Resolving that is a resolver's job, and § 10.4 keeps composition declarative. + +The fix belongs to the author, and it is usually a better factoring. Extract the +part that is genuinely shared into its own fragment and include it once at each +level that needs it, rather than composing a whole package that carries it: + +```text +review/change +├── style → include style/house +└── routine → include review/commit-routine (no style of its own) +``` + +A tool that renders a package SHOULD report when an included value will render +more than once — whether because two inputs resolved to the same text, or +because one included value contains another — since the author usually did not +intend it. + CPF does **not** define template inheritance. A package does not extend another, override its sections, or inherit its inputs. Composition is by reference only, for four reasons drawn from this specification and from diff --git a/reference/README.md b/reference/README.md index 67ea163..4632366 100644 --- a/reference/README.md +++ b/reference/README.md @@ -79,6 +79,10 @@ rather than resolved by guessing. print qualified `:` references. - `include` defaults are satisfied (inclusion is deterministic); `derive` defaults are reported, not run. Inclusion cycles are detected and named. +- `resolve`, `render` and `eval` warn when an included value will render more + than once — two inputs including the same thing, or an included package + inheriting an outer input of the same name. Nothing is deduplicated; the fix + is a better factoring. - `eval` runs the deterministic render checks of any eval declaring the `canned-prompts/eval-rubric/v0.1` schema, and reports output criteria as declared but not run. Unrecognized schemas are skipped, not rejected. A diff --git a/reference/canned_prompts.py b/reference/canned_prompts.py index 4aa2896..953daf4 100755 --- a/reference/canned_prompts.py +++ b/reference/canned_prompts.py @@ -783,6 +783,45 @@ def report_missing_dependencies(catalog: Path, manifest: dict[str, Any]) -> None ) +def duplicate_inclusions(resolution: Resolution) -> list[str]: + """Included values that will render more than once (§ 10.4). + + Two ways this happens, and both are the same defect to an author: + + - two inputs include the same dependency, so the values are equal; + - an included package inherits an outer input of the same name and renders + it again, so one included value *contains* another. + + The second is the common one and is easy to miss: nothing was included + twice, but the text appears twice all the same. + """ + included = { + name: str(resolution.values[name]) + for name, origin in resolution.origins.items() + if origin.startswith("included from") and name in resolution.values + } + duplicated: set[str] = set() + for name, value in included.items(): + if not value.strip(): + continue + for other, other_value in included.items(): + if other != name and (value == other_value or value in other_value): + duplicated.add(name) + return sorted(duplicated) + + +def report_repeated_inclusions(resolution: Resolution) -> None: + repeated = duplicate_inclusions(resolution) + if repeated: + print( + "rendered more than once: " + + ", ".join(repeated) + + " — inclusion is textual and is not deduplicated (§ 10.4); " + "extract the shared part into its own fragment", + file=sys.stderr, + ) + + def report_skipped(skipped: list[str]) -> None: if skipped: print( @@ -1154,7 +1193,9 @@ def cmd_render(args: argparse.Namespace) -> None: manifest = validate_package(package_dir) template_path = safe_relative_file(package_dir, manifest["template"], "template") raw = supplied_values(args.set_values) - resolution = resolve_inputs(manifest, raw, composer=CatalogComposer(catalog)) + composer = CatalogComposer(catalog) + resolution = resolve_inputs(manifest, raw, composer=composer) + report_repeated_inclusions(resolution) template = template_path.read_text(encoding="utf-8") used = set(PLACEHOLDER_RE.findall(template)) @@ -1176,7 +1217,9 @@ def cmd_resolve(args: argparse.Namespace) -> None: package_dir, _ = resolve_installed(catalog, args.id, args.version) manifest = validate_package(package_dir) raw = supplied_values(args.set_values) - resolution = resolve_inputs(manifest, raw, composer=CatalogComposer(catalog)) + composer = CatalogComposer(catalog) + resolution = resolve_inputs(manifest, raw, composer=composer) + report_repeated_inclusions(resolution) if args.json: print( @@ -1291,9 +1334,9 @@ def cmd_eval(args: argparse.Namespace) -> None: if spec.get("example") else {} ) - resolution = resolve_inputs( - manifest, values, composer=CatalogComposer(catalog) - ) + composer = CatalogComposer(catalog) + resolution = resolve_inputs(manifest, values, composer=composer) + report_repeated_inclusions(resolution) missing = sorted(set(PLACEHOLDER_RE.findall(template)) - set(resolution.values)) rendered = "" if missing else render_template(template, resolution.values) if missing: diff --git a/reference/tests/test_canned_prompts.py b/reference/tests/test_canned_prompts.py index 957de73..f96d599 100644 --- a/reference/tests/test_canned_prompts.py +++ b/reference/tests/test_canned_prompts.py @@ -867,3 +867,50 @@ def test_bad_index_format_is_rejected(tmp_path: Path) -> None: (store / "index.yaml").write_text("format: something/else\nentries: []\n", encoding="utf-8") with pytest.raises(cp.CannedPromptError, match="unsupported index format"): cp.read_index(store) + + +# --- inclusion diamonds (§ 10.4) --- + +def test_no_duplicate_when_inclusions_differ() -> None: + resolution = cp.Resolution( + values={"a": "alpha text", "b": "beta text"}, + origins={"a": "included from x/a", "b": "included from x/b"}, + ) + assert cp.duplicate_inclusions(resolution) == [] + + +def test_two_inputs_including_the_same_thing_is_flagged() -> None: + resolution = cp.Resolution( + values={"a": "shared text", "b": "shared text"}, + origins={"a": "included from x/shared", "b": "included from x/shared"}, + ) + assert cp.duplicate_inclusions(resolution) == ["a", "b"] + + +def test_nested_inheritance_duplication_is_flagged() -> None: + """The common case: an included package inherits an outer input by name.""" + resolution = cp.Resolution( + values={"conventions": "RULES", "routine": "step one\nRULES\nstep two"}, + origins={ + "conventions": "included from team/conventions", + "routine": "included from team/routine", + }, + ) + assert cp.duplicate_inclusions(resolution) == ["conventions"] + + +def test_supplied_values_are_not_flagged() -> None: + """Only inclusions are checked; a caller repeating text is their business.""" + resolution = cp.Resolution( + values={"a": "same", "b": "same"}, + origins={"a": "supplied", "b": "supplied"}, + ) + assert cp.duplicate_inclusions(resolution) == [] + + +def test_empty_inclusion_is_not_flagged() -> None: + resolution = cp.Resolution( + values={"a": "", "b": "anything"}, + origins={"a": "included from x/empty", "b": "included from x/b"}, + ) + assert cp.duplicate_inclusions(resolution) == [] diff --git a/workplans/CANP-WP-0005-inclusion-diamond.md b/workplans/CANP-WP-0005-inclusion-diamond.md index 81f14ec..e065872 100644 --- a/workplans/CANP-WP-0005-inclusion-diamond.md +++ b/workplans/CANP-WP-0005-inclusion-diamond.md @@ -4,7 +4,7 @@ type: workplan title: "Document and detect inclusion diamonds" domain: agents repo: canned-prompts -status: proposed +status: finished owner: codex topic_slug: practice created: "2026-09-06" @@ -20,7 +20,7 @@ Residual from `CANP-WP-0004` (`origin: residual`, `origin_ref: CANP-WP-0004`). ```task id: CANP-WP-0005-T01 -status: todo +status: done priority: medium state_hub_task_id: "74234a7a-c3bd-5bf7-a40d-21ddf208cfb5" ``` @@ -47,3 +47,28 @@ Two questions, and the second depends on the first: purpose. Leaning: document it now, warn at validation time, and do not deduplicate. + +## Outcome + +Done as leaned, with one correction found by testing. + +**The documented mechanism was wrong at first.** § 10.4 was written describing a +dependency "reached by two paths" through two `include` chains, and the detector +counted composer calls to match. Tested against the real case that motivated the +task — `helix/repo-advance` — and it did not fire, even though the conventions +block still rendered twice. + +The actual mechanism is different and more subtle. `helix/commit-sync` declares +its own `conventions` input; when composed, resolution passes the including +package's already-resolved values down by name (§ 10.4 inheritance), so it +*inherits* the outer text rather than resolving its own include — and renders it +a second time. Nothing was included twice. The text appeared twice regardless. + +Both mechanisms are now documented with worked diagrams, and +`duplicate_inclusions` detects both: included values that are equal, and an +included value contained within another. `resolve`, `render` and `eval` warn; +nothing is deduplicated. + +Verified by reintroducing the diamond in a scratch copy of `repo-advance` — it +warns — and confirming the shipped factored collection stays silent. +Tests 90 → 95.